[24.0 backport] daemon: daemon.prepareMountPoints(): fix panic if mount is not a volume#45903
Merged
neersighted merged 1 commit intomoby:24.0from Jul 7, 2023
Merged
Conversation
The daemon.lazyInitializeVolume() function only handles restoring Volumes if a Driver is specified. The Container's MountPoints field may also contain other kind of mounts (e.g., bind-mounts). Those were ignored, and don't return an error; https://github.com/moby/moby/blob/1d9c8619cded4657af1529779c5771127e8ad0e7/daemon/volumes.go#L243-L252C2 However, the prepareMountPoints() assumed each MountPoint was a volume, and logged an informational message about the volume being restored; https://github.com/moby/moby/blob/1d9c8619cded4657af1529779c5771127e8ad0e7/daemon/mounts.go#L18-L25 This would panic if the MountPoint was not a volume; github.com/docker/docker/daemon.(*Daemon).prepareMountPoints(0xc00054b7b8?, 0xc0007c2500) /root/rpmbuild/BUILD/src/engine/.gopath/src/github.com/docker/docker/daemon/mounts.go:24 +0x1c0 github.com/docker/docker/daemon.(*Daemon).restore.func5(0xc0007c2500, 0x0?) /root/rpmbuild/BUILD/src/engine/.gopath/src/github.com/docker/docker/daemon/daemon.go:552 +0x271 created by github.com/docker/docker/daemon.(*Daemon).restore /root/rpmbuild/BUILD/src/engine/.gopath/src/github.com/docker/docker/daemon/daemon.go:530 +0x8d8 panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x564e9be4c7c0] This issue was introduced in 647c2a6 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit a490248) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Member
Author
|
@cpuguy83 @neersighted PTAL |
cpuguy83
approved these changes
Jul 7, 2023
neersighted
approved these changes
Jul 7, 2023
|
I think it might be useful to add some test using bind-mounts that might catch if this or anything similar gets wrong again. But I am not familiar with your tests, and how/if this would be feasible, I just want to suggest it, in case this slipped through while you worked on resolving this issue. |
Member
|
Thanks @r4co0n, the change does add a test for the issue. |
|
@cpuguy83, I concur, I really don't know why I did not notice the relevant test, thank you for clarifying. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The daemon.lazyInitializeVolume() function only handles restoring Volumes if a Driver is specified. The Container's MountPoints field may also contain other kind of mounts (e.g., bind-mounts). Those were ignored, and don't return an error; https://github.com/moby/moby/blob/1d9c8619cded4657af1529779c5771127e8ad0e7/daemon/volumes.go#L243-L252C2
However, the prepareMountPoints() assumed each MountPoint was a volume, and logged an informational message about the volume being restored;
moby/daemon/mounts.go
Lines 18 to 25 in 1d9c861
This would panic if the MountPoint was not a volume;
This issue was introduced in 647c2a6
(cherry picked from commit a490248)
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)