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

Close sync pipe explicitly in exec #4192

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Feb 6, 2024

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:

# bats tests/integration/exec.bats 
...
 ✗ RUNC_DMZ=legacy runc exec [execve error]
   (in test file tests/integration/exec.bats, line 340)
     `[ ${#lines[@]} -eq 1 ]' failed
   runc spec (status=0):
   
   runc run -d --console-socket /tmp/bats-run-77CvQx/runc.JurXM2/tty/sock test_busybox (status=0):
   
   runc exec -t test_busybox /run.sh (status=255):
   writing sync procError: write sync: bad file descriptor
   exec /run.sh: no such file or directory

@kolyshkin
Copy link
Contributor

@lifubang I did that initially (instead of what I do in #4183) , and then I thought there is actually no need to close it. It's a socket.

@lifubang
Copy link
Member Author

lifubang commented Feb 7, 2024

and then I thought there is actually no need to close it. It's a socket.

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 runc start and runc exec if there is an error returned by execve, because we took two phrases to start a container, so we need to close the sync pipe to tell the runc create process to exit(maybe we can change to other ways), this is not the same as runc exec, so it causes two different error msgs for execve.

Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM

@lifubang lifubang force-pushed the feat-ClosePipeInExec branch 2 times, most recently from b79fc96 to d4b6979 Compare February 7, 2024 11:55
Signed-off-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
@kolyshkin
Copy link
Contributor

@lifubang we can merge it if you can remove last commit.

Copy link
Contributor

@kolyshkin kolyshkin left a 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)

@lifubang
Copy link
Member Author

lifubang commented Feb 8, 2024

if you can remove last commit.

If we remove the last commit, the tests for centos will fail.

@kolyshkin
Copy link
Contributor

If we remove the last commit, the tests for centos will fail.

I know, but we're fixing those in #4193

@lifubang
Copy link
Member Author

lifubang commented Feb 8, 2024

As go has released v1.22.0, so there is no 1.20.x in https://go.dev/dl/?mode=json anymore.
Please see #4193
If we split the last commit to a seperate PR, it also will fail because of this PR.
So I think we should combine them together to one PR.

@kolyshkin
Copy link
Contributor

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.

@lifubang
Copy link
Member Author

lifubang commented Feb 8, 2024

@opencontainers/runc-maintainers PTAL, the cirrus CI errors will be fixed in #4198 .

@kolyshkin kolyshkin merged commit 7c004d8 into opencontainers:main Feb 8, 2024
42 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants