Skip to content

daemon: daemon.prepareMountPoints(): fix panic if mount is not a volume#45902

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:fix_volume_npe
Jul 7, 2023
Merged

daemon: daemon.prepareMountPoints(): fix panic if mount is not a volume#45902
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:fix_volume_npe

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Jul 7, 2023 •
edited
Loading

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

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

@thaJeztah thaJeztah added this to the 25.0.0 milestone Jul 7, 2023
@thaJeztah thaJeztah requested review from cpuguy83 and neersighted July 7, 2023 13:12
@thaJeztah thaJeztah mentioned this pull request Jul 7, 2023
Closed
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jul 7, 2023

Ah, dang failing on rootless;

=== FAIL: amd64.integration.daemon TestLiveRestore/volume_references/container_with_bind-mounts (0.00s)
    daemon_test.go:449: assertion failed: error is not nil: Error response from daemon: invalid mount config for type "bind": stat /tmp/TestLiveRestorevolume_referencescontainer_with_bind-mounts645691285/001: permission denied
        --- FAIL: TestLiveRestore/volume_references/container_with_bind-mounts (0.00s)

@thaJeztah
daemon: daemon.prepareMountPoints(): fix panic if mount is not a volume
a490248 
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>
@thaJeztah thaJeztah mentioned this pull request Jul 7, 2023
Merged
neersighted
neersighted approved these changes Jul 7, 2023
View reviewed changes
daemon/mounts.go
if err := daemon.lazyInitializeVolume(container.ID, config); err != nil {
return err
}
if config.Volume == nil {
Copy link
Copy Markdown
Member

@neersighted neersighted Jul 7, 2023

Choose a reason for hiding this comment

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

This looks to be the best fix for now; though we should probably come up with a better interface for mountpoints (e.g. a method to check if they're volume-like/convert them into a volume), since they currently feel like a very leaky abstraction.

@cpuguy83 cpuguy83 merged commit 7d548c6 into moby:master Jul 7, 2023
@thaJeztah thaJeztah deleted the fix_volume_npe branch July 7, 2023 14:38
This was referenced Jul 7, 2023
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Jul 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@neersighted neersighted neersighted approved these changes

Assignees

No one assigned

Labels

area/daemon Core Engine area/volumes Volumes kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review

Projects

None yet

Milestone

25.0.0

4 participants

@thaJeztah @cpuguy83 @neersighted @corhere