Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[24.0 backport] daemon: daemon.prepareMountPoints(): fix panic if mount is not a volume #45903

Merged
merged 1 commit into from Jul 7, 2023

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 7, 2023

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

if err := daemon.lazyInitializeVolume(container.ID, config); err != nil {
return err
}
if alive {
log.G(context.TODO()).WithFields(logrus.Fields{
"container": container.ID,
"volume": config.Volume.Name(),
}).Debug("Live-restoring volume for alive container")

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

(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)

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>
@thaJeztah thaJeztah added this to the 24.0.4 milestone Jul 7, 2023
@thaJeztah thaJeztah marked this pull request as ready for review July 7, 2023 14:38
@thaJeztah
Copy link
Member Author

@cpuguy83 @neersighted PTAL

@neersighted neersighted merged commit 4ffc614 into moby:24.0 Jul 7, 2023
101 checks passed
@r4co0n
Copy link

r4co0n commented 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.

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 7, 2023

Thanks @r4co0n, the change does add a test for the issue.

@r4co0n
Copy link

r4co0n commented Jul 7, 2023

@cpuguy83, I concur, I really don't know why I did not notice the relevant test, thank you for clarifying.

Sign up for free to 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.

None yet

4 participants