Skip to content

[27.x backport] plugins: don't panic on Close if PluginServer nil - #5337

Merged
thaJeztah merged 1 commit into
docker:27.xfrom
vvoland:5327-27.x
Aug 9, 2024
Merged

[27.x backport] plugins: don't panic on Close if PluginServer nil#5337
thaJeztah merged 1 commit into
docker:27.xfrom
vvoland:5327-27.x

Conversation

@vvoland

@vvolandvvoland commented Aug 9, 2024

Copy link
Copy Markdown
Collaborator

- What I did

We had a report in fixes#5317 of a panic at

pl.mu.Lock()

Looks like this will happen if the call to create a PluginServer fails and returns nil, since we're not checking for nilness before attempting to call Close:

cli/cmd/docker/docker.go

Lines 247 to 251 in 87c6624

deferfunc() {
// Close the server when plugin execution is over, so that in case
// it's still open, any sockets on the filesystem are cleaned up.
_=srv.Close()
}()

_=srv.Close()

- How I did it

Added a nil check to PluginServer.Close(). We could also have addressed this at the caller sites, but this seems more robust for little cost.

- How to verify it

- Description for the changelog

Fixed issue where main CLI process would sometimes panic while running a plugin.

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(cherry picked from commit 9c44806)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvolandvvoland added this to the 27.1.2 milestone Aug 9, 2024
@vvolandvvoland self-assigned this Aug 9, 2024
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.05%. Comparing base (40707e1) to head (984ef90).
Report is 1 commits behind head on 27.x.

Additional details and impacted files
@@ Coverage Diff @@## 27.x #5337 +/- ##
==========================================
- Coverage 61.49% 61.05% -0.44% 
==========================================
Files 299 296 -3 Lines 20867 20861 -6 ==========================================
- Hits 12832 12737 -95 - Misses 7120 7207 +87 - Partials 915 917 +2 

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
thaJeztah merged commit f62c68e into docker:27.xAug 9, 2024
renovateBot added a commit to earthly/dind that referenced this pull request Aug 19, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)
This PR contains the following updates:
| Package | Update | Change |
|---|---|---|
| [docker/docker](https://togithub.com/docker/docker) | patch | `27.1.1`
-> `27.1.2` |
---
### Release Notes
<details>
<summary>docker/docker (docker/docker)</summary>
### [`v27.1.2`](https://togithub.com/moby/moby/releases/tag/v27.1.2)
[Compare
Source](https://togithub.com/docker/docker/compare/v27.1.1...v27.1.2)
#### 27.1.2
For a full list of pull requests and changes in this release, refer to
the relevant GitHub milestones:
- [docker/cli, 27.1.2
milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.1.2)
- [moby/moby, 27.1.2
milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.1.2)
- Deprecated and removed features, see [Deprecated
Features](https://togithub.com/docker/cli/blob/v27.1.2/docs/deprecated.md).
- Changes to the Engine API, see [API version
history](https://togithub.com/moby/moby/blob/v27.1.2/docs/api/version-history.md).
##### Bug fixes and enhancements
- Fix a regression that could result in a `ResourceExhausted desc =
grpc: received message larger than max` error when building from a large
Dockerfile. [moby/moby#48245](https://togithub.com/moby/moby/pull/48245)
- CLI: Fix `docker attach` printing a spurious `context cancelled` error
message. [docker/cli#5296](https://togithub.com/docker/cli/pull/5296)
- CLI: Fix `docker attach` exiting on `SIGINT` instead of forwarding the
signal to the container and waiting for it to exit.
[docker/cli#5302](https://togithub.com/docker/cli/pull/5302)
- CLI: Fix `--device-read-bps` and `--device-write-bps` options not
taking effect.
[docker/cli#5339](https://togithub.com/docker/cli/pull/5339)
- CLI: Fix a panic happening in some cases while running a plugin.
[docker/cli#5337](https://togithub.com/docker/cli/pull/5337)
##### Packaging updates
- Update BuildKit to
[v0.15.1](https://togithub.com/moby/buildkit/releases/tag/v0.15.1).
[moby/moby#48246](https://togithub.com/moby/moby/pull/48246)
- Update Buildx to
[v0.16.2](https://togithub.com/docker/buildx/releases/tag/v0.16.2).
[docker/docker-ce-packaging#1043](https://togithub.com/docker/docker-ce-packaging/pull/1043)
- Update Go runtime to 1.21.13.
[moby/moby#48301](https://togithub.com/moby/moby/pull/48301),
[docker/cli#5325](https://togithub.com/docker/cli/pull/5325),
[docker/docker-ce-packaging#1046](https://togithub.com/docker/docker-ce-packaging/pull/1046)
- Remove unused `docker-proxy.exe` binary from Windows packages.
[docker/docker-ce-packaging#1045](https://togithub.com/docker/docker-ce-packaging/pull/1045)
</details>
---
### Configuration
📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge
- At any time (no schedule defined).
🚦 **Automerge**: Enabled.
♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.
🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.
---
- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box
---
This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/earthly/dind).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUiXX0=-->
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovateBot added a commit to earthly/dind that referenced this pull request Aug 19, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)
This PR contains the following updates:
| Package | Update | Change |
|---|---|---|
| [docker/docker](https://togithub.com/docker/docker) | patch | `27.1.1`
-> `27.1.2` |
---
### Release Notes
<details>
<summary>docker/docker (docker/docker)</summary>
### [`v27.1.2`](https://togithub.com/moby/moby/releases/tag/v27.1.2)
[Compare
Source](https://togithub.com/docker/docker/compare/v27.1.1...v27.1.2)
#### 27.1.2
For a full list of pull requests and changes in this release, refer to
the relevant GitHub milestones:
- [docker/cli, 27.1.2
milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A27.1.2)
- [moby/moby, 27.1.2
milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A27.1.2)
- Deprecated and removed features, see [Deprecated
Features](https://togithub.com/docker/cli/blob/v27.1.2/docs/deprecated.md).
- Changes to the Engine API, see [API version
history](https://togithub.com/moby/moby/blob/v27.1.2/docs/api/version-history.md).
##### Bug fixes and enhancements
- Fix a regression that could result in a `ResourceExhausted desc =
grpc: received message larger than max` error when building from a large
Dockerfile. [moby/moby#48245](https://togithub.com/moby/moby/pull/48245)
- CLI: Fix `docker attach` printing a spurious `context cancelled` error
message. [docker/cli#5296](https://togithub.com/docker/cli/pull/5296)
- CLI: Fix `docker attach` exiting on `SIGINT` instead of forwarding the
signal to the container and waiting for it to exit.
[docker/cli#5302](https://togithub.com/docker/cli/pull/5302)
- CLI: Fix `--device-read-bps` and `--device-write-bps` options not
taking effect.
[docker/cli#5339](https://togithub.com/docker/cli/pull/5339)
- CLI: Fix a panic happening in some cases while running a plugin.
[docker/cli#5337](https://togithub.com/docker/cli/pull/5337)
##### Packaging updates
- Update BuildKit to
[v0.15.1](https://togithub.com/moby/buildkit/releases/tag/v0.15.1).
[moby/moby#48246](https://togithub.com/moby/moby/pull/48246)
- Update Buildx to
[v0.16.2](https://togithub.com/docker/buildx/releases/tag/v0.16.2).
[docker/docker-ce-packaging#1043](https://togithub.com/docker/docker-ce-packaging/pull/1043)
- Update Go runtime to 1.21.13.
[moby/moby#48301](https://togithub.com/moby/moby/pull/48301),
[docker/cli#5325](https://togithub.com/docker/cli/pull/5325),
[docker/docker-ce-packaging#1046](https://togithub.com/docker/docker-ce-packaging/pull/1046)
- Remove unused `docker-proxy.exe` binary from Windows packages.
[docker/docker-ce-packaging#1045](https://togithub.com/docker/docker-ce-packaging/pull/1045)
</details>
---
### Configuration
📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge
- At any time (no schedule defined).
🚦 **Automerge**: Enabled.
♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.
🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.
---
- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box
---
This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/earthly/dind).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUiXX0=-->
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@vvoland@codecov-commenter@thaJeztah@laurazard