-
Notifications
You must be signed in to change notification settings - Fork 70
fix unshare infinite loop without CGO_ENABLED #590
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
base: main
Are you sure you want to change the base?
Conversation
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6637 |
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.
Thanks!
I can’t see how this can possibly work. AFAICT the code with _Containers-pid-pipe and _Containers-continue-pipe exists in C became we need it to run (and give all the process environment changes in unshare.Cmd.Start an opportunity to happen) before the Go runtime starts creating any OS-level threads.
We can mimic the child process’ behavior in Go but I don’t see how we can reliably achieve the same outcome.
Cc: @giuseppe
giuseppe
left a comment
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.
how is this supposed to work?
Is this AI-generated?
storage/pkg/unshare/unshare_nocgo.go
Outdated
| attr.AmbientCaps = []uintptr{ | ||
| unix.CAP_CHOWN, | ||
| unix.CAP_DAC_OVERRIDE, | ||
| unix.CAP_DAC_READ_SEARCH, | ||
| unix.CAP_FOWNER, | ||
| unix.CAP_FSETID, | ||
| unix.CAP_SYS_ADMIN, | ||
| } | ||
| } |
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.
I've not tested this PR but this won't work, we need all caps because we launch containers from this environment (and they don't need to be in the ambient set)
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.
I've compared processes with C code and those without, and you're right—the C process does include all capabilities. However, I'm not entirely sure if adding all privileges is the correct decision. The reason I added these capabilities in my code was that when using storage, it directly attempted to delete a folder with permissions r-xr-xr-x, which resulted in a Permission deniederror. Adding the capabilities was my workaround to resolve this permission issue. Based on your suggestion, I will proceed with adding all the capabilities.
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.
I haven't found any alternative methods to add capabilities other than using the ambient set. Do you have any suggestions?
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.
When you create a user namespace you automatically get them. Is Go dropping them?
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.
Based on my testing, I found that child processes will not have capabilities unless the ambient method is used. I collected relevant child process information from the /proc/<pid>/status file under three scenarios: using CGO, not using CGO without setting ambient capabilities, and not using CGO with ambient capabilities set.
Using CGO:
CapInh: 0000000000000000
CapPrm: 000001ffffffffff
CapEff: 000001ffffffffff
CapBnd: 000001ffffffffff
CapAmb: 0000000000000000
Not using CGO and not setting ambient capabilities:
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 000001ffffffffff
CapAmb: 0000000000000000
Not using CGO but setting ambient capabilities:
CapInh: 000001ffffffffff
CapPrm: 000001ffffffffff
CapEff: 000001ffffffffff
CapBnd: 000001ffffffffff
CapAmb: 000001ffffffffff
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.
This affects also processes executed by Podman, could you investigate why this is happening? An alternative would be to drop the ambient caps once we are in the namespace
I needed to use statically compiled Skopeo and Buildah for container image migration tasks in an offline environment. I compiled the necessary executables using commands similar to:
However, I encountered an issue where the unsharepackage did not function properly when CGO_ENABLED=0. While Skopeo worked correctly in most scenarios, operations involving containers-storage(which call unshare.MaybeReexecUsingUserNamespace, e.g. To resolve this, I modified the code to directly pass |
None of this addresses the concern that doing any such changes after the Go runtime starts is too late. What am I missing? (Giuseppe?) The conversation in #160 was AFAICT saying that to resolve this
I realize that does not help your use case but that doesn’t mean that we can ignore the constraints (if there indeed are any). |
|
Under Giuseppe guidance, I re-investigated the implementation of unshare. After reading the source code related to golang's The golang The current approach uses the The main process uses |
Signed-off-by: lyp256 <lyp256@qq.com>
giuseppe
left a comment
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.
tested locally.
LGTM
fix unshare.MaybeReexecUsingUserNamespace() infinite loop without CGO_ENABLED.
fixes: #160