-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Close sync pipe explicitly in exec #4192
Conversation
Yes, maybe there is no need to close it at this time, but my thought is that we should get the same error msg for |
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
b79fc96
to
d4b6979
Compare
Signed-off-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
1e99d69
to
52432d7
Compare
@lifubang we can merge it if you can remove last commit. |
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.
LGMT (except the last commit)
If we remove the last commit, the tests for centos will fail. |
I know, but we're fixing those in #4193 |
As go has released v1.22.0, so there is no 1.20.x in https://go.dev/dl/?mode=json anymore. |
I mean, a maintainer can merge bypassing the branch protection rules, and that's what I'm going to do. Unfortunately we're slow to fix CI and the issues are piling up. |
52432d7
to
bc4a869
Compare
@opencontainers/runc-maintainers PTAL, the cirrus CI errors will be fixed in #4198 . |
Fix #4183
To simplify the implemention, we can keep it the same as in
standard_init_linux.go
.Recent CVE-2024-21626 fix (commit f2f1621) broke a recently added test (commit 0bc4732, #4173) because if exec fails, runc is unable to communicate the error back to the parent.
Let's not close syncPipe.
This fixes the following test failure: