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

never send procError after the socket closed #4173

Merged
merged 2 commits into from
Jan 27, 2024

Conversation

lifubang
Copy link
Member

We have moved the error msg display position in #3385, I think it's reasonable to display the container init msg in one place, but in this PR, we changed an error ignore behavior in https://github.com/opencontainers/runc/blob/main/libcontainer/init_linux.go#L112-L113 .

Yes, we should display the error before procReady has sent, but we should ignore after that. So I think we can add a field to indicate whether we have sent procReady or not in child process. It's not needed for the parent process, but we can reuse it to remove a local var seenProcReady.

Fix #4171

@lifubang lifubang mentioned this pull request Jan 16, 2024
20 tasks
@rata
Copy link
Member

rata commented Jan 16, 2024

@lifubang can we add an integration test for this? So the next refactor doesn't regress on this.

@lifubang
Copy link
Member Author

can we add an integration test for this? So the next refactor doesn't regress on this.

Good idea, added two tests. Thanks.

libcontainer/sync_unix.go Outdated Show resolved Hide resolved
libcontainer/process_linux.go Outdated Show resolved Hide resolved
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

I think atomic.Bool is nicer for this than using mutexes, but I don't have a strong opinion.

libcontainer/sync_unix.go Outdated Show resolved Hide resolved
libcontainer/sync_unix.go Outdated Show resolved Hide resolved
libcontainer/init_linux.go Outdated Show resolved Hide resolved
libcontainer/sync_unix.go Outdated Show resolved Hide resolved
@lifubang lifubang force-pushed the fix-syncPipeClose branch 2 times, most recently from e92ac5d to 79f3d6a Compare January 24, 2024 16:25
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the nitpicks

Signed-off-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
@AkihiroSuda AkihiroSuda merged commit 2dfc2fe into opencontainers:main Jan 27, 2024
45 checks passed
@lifubang lifubang changed the title never send procError to parent process after sent procReady never send procError after the socket closed Jan 28, 2024
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.

writing sync procError: write sync: file already closed
5 participants