Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
cyphar merged 2 commits intoopencontainers:mainfromkolyshkin:fix-close
Oct 15, 2025

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkinkolyshkin commentedOct 13, 2025
edited
Loading

The(*setns).start is 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.

@kolyshkinkolyshkin added the backport/1.4-todoA PR in main branch which needs to backported to release-1.4 labelOct 13, 2025
@kolyshkin
Copy link
ContributorAuthor

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.

Copy link

CopilotAI left a 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 missingp.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.

@kolyshkinkolyshkin added this to the1.4.0 milestoneOct 13, 2025
@kolyshkinkolyshkinforce-pushed thefix-close branch 2 times, most recently from0c18cce to2e5864cCompareOctober 14, 2025 18:40
Copy link

CopilotAI left a 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
Copy link
ContributorAuthor

Updated, and rebased#4928 (which also serves as a test for the fix in here).

kolyshkinand others added2 commitsOctober 14, 2025 11:48
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>
Copy link

CopilotAI left a 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.

@cypharcyphar merged commitef90082 intoopencontainers:mainOct 15, 2025
36 checks passed
@kolyshkinkolyshkin added backport/1.4-doneA PR in main branch which has been backported to release-1.4 and removed backport/1.4-todoA PR in main branch which needs to backported to release-1.4 labelsOct 15, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

Copilot code reviewCopilotCopilot left review comments

@lifubanglifubanglifubang approved these changes

@cypharcypharcyphar approved these changes

@ratarataAwaiting requested review from rata

Assignees

No one assigned

Labels

backport/1.4-doneA PR in main branch which has been backported to release-1.4

Projects

None yet

Milestone

1.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@kolyshkin@lifubang@cyphar

[8]ページ先頭

©2009-2025 Movatter.jp