- Notifications
You must be signed in to change notification settings - Fork2.2k
libct: close child fds on prepareCgroupFD error#4930
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
kolyshkin commentedOct 13, 2025
Technically, this is just leaking unclosed fds if prepareCgroupFD returns an error, which is a minor issue. But together with changes in#4928 this creates a case when runc waits on log forwarder forever because the other side of the log pipe is never closed. |
Uh oh!
There was an error while loading.Please reload this page.
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.
Pull Request Overview
This PR fixes a resource leak bug where child file descriptors were not being properly closed whenprepareCgroupFD() fails in the(*setns).start() method. The fix ensures that child fds are closed consistently in all error paths, maintaining the expected behavior that child fds are closed either when the child starts successfully or when an error occurs.
- Adds missing
p.comm.closeChild()call in theprepareCgroupFD()error path - Ensures consistent resource cleanup across all error scenarios in the start method
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
0c18cce to2e5864cCompareThere 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.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
kolyshkin commentedOct 14, 2025
Updated, and rebased#4928 (which also serves as a test for the fix in here). |
The (*setns).start is supposed to close child fds once the child hasstarted, or upon an error. Commit5af4dd4 added a bug -- child fdsare not closed if prepareCgroupFD fails.Fix by adding a missing call to closeChild.I'm not sure how to write a good test case for it. Found when workingon PR 4928 (and tested in there).Fixes:5af4dd4Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Factor startWithCgroupFD out of start to reduce the start complexity.This also implements a more future-proof way of calling p.comm.closeChild.Co-authored-by: lifubang <lifubang@acmcoder.com>Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ef90082 intoopencontainers:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
The
(*setns).startis supposed to close child fds once the child has started, or upon returning an error.There was no code to return an error before calling start, but commit5af4dd4 added it, together with
a bug -- child fds are not closed if prepareCgroupFD fails.
I'm not sure ifhow to add a good test case for it. Found when working on PR#4928 (which modified the code
to read the child logs even when start() fails).
Fixes:5af4dd4 / PR#4812.
This PR also includes the refactoring of start to avoid similar problems in the future.