-
Notifications
You must be signed in to change notification settings - Fork 246
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
canUseShifting can segfault #932
Conversation
@giuseppe @edsantiago PTAL |
LGTM, in principle, but do you have any idea what could've led to |
No idea, @giuseppe needs to look at this, This change just makes it safer. |
what do you think if we just move the check later in diff --git a/store.go b/store.go
index 759407c63..8331fafde 100644
--- a/store.go
+++ b/store.go
@@ -2666,6 +2666,10 @@ func (s *store) mount(id string, options drivers.MountOpts) (string, error) {
s.lastLoaded = time.Now()
}
+ if options.UidMaps != nil || options.GidMaps != nil {
+ options.DisableShifting = !s.canUseShifting(options.UidMaps, options.GidMaps)
+ }
+
if rlstore.Exists(id) {
return rlstore.Mount(id, options)
}
@@ -2706,7 +2710,6 @@ func (s *store) Mount(id, mountLabel string) (string, error) {
options.Volatile = v.(bool)
}
}
- options.DisableShifting = !s.canUseShifting(container.UIDMap, container.GIDMap)
}
return s.mount(id, options)
} Otherwise we may end up using shifting even if the driver doesn't support it |
@giuseppe Looking at this, I am not sure this should be handled at the store level at all. I think we should just remove this and do it at the driver level. Is it your intention to allow users in their storage.conf to specify to disable shifting? |
I think the store needs to know when shifting is supported by the underlying driver. When it is not supported it needs to create a copy of the image with the shifted IDs. |
Fixes: containers/podman#10535 Signed-off-by: Daniel J Walsh <[email protected]>
@giuseppe PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@saschagrunert @vrothberg PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes: containers/podman#10535
Signed-off-by: Daniel J Walsh [email protected]