libct: close child fds on prepareCgroupFD error #4930
Conversation
kolyshkin
added
the
backport/1.4-todo
|
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. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a resource leak bug where child file descriptors were not being properly closed when prepareCgroupFD() 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 or learn how to get started.
kolyshkin
force-pushed
the
fix-close
branch
2 times, most recently
from
0c18cce to
2e5864c
Compare
There was a problem hiding this comment.
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 or learn how to get started.
|
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 has started, or upon an error. Commit 5af4dd4 added a bug -- child fds are 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 working on PR 4928 (and tested in there). Fixes: 5af4dd4 Signed-off-by: Kir Kolyshkin <[email protected]>
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 <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>
There was a problem hiding this comment.
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 or learn how to get started.
kolyshkin
added
backport/1.4-done
3 participants
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 commit 5af4dd4 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.