Skip to content

fix(supervisor): manage received file descriptor ownership - #783

Open
Park-Jiyeonn wants to merge 1 commit into
containerd:mainfrom
Park-Jiyeonn:fix/supervisor-fd-lifecycle
Open

fix(supervisor): manage received file descriptor ownership#783
Park-Jiyeonn wants to merge 1 commit into
containerd:mainfrom
Park-Jiyeonn:fix/supervisor-fd-lifecycle

Conversation

@Park-Jiyeonn

Copy link
Copy Markdown
Contributor

Overview

Close superseded state file descriptors and release descriptors received on invalid SCM_RIGHTS messages. Validate control-message truncation and require exactly one descriptor so malformed state transfers cannot leak resources or leave ambiguous ownership.

Change Type

Please select the type of change your pull request relates to:

  • Bug Fix
  • Feature Addition
  • Documentation Update
  • Code Refactoring
  • Performance Improvement
  • Other (please describe)

@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.27027% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 25.84%. Comparing base (18e7e6a) to head (6d5836d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/supervisor/supervisor.go 70.27% 6 Missing and 5 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #783      +/-   ##
==========================================
- Coverage   26.01%   25.84%   -0.17%     
==========================================
  Files         133      134       +1     
  Lines       12711    12833     +122     
==========================================
+ Hits         3307     3317      +10     
- Misses       8998     9110     +112     
  Partials      406      406              
Files with missing lines Coverage Δ
pkg/supervisor/supervisor.go 70.44% <70.27%> (+1.53%) ⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

}
}

func recv(uc *net.UnixConn) (data []byte, fd int, retErr error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A small suggestion: unify the return value of fd for error paths in recv to fd (default value -1).

Close superseded state file descriptors and release descriptors received on invalid SCM_RIGHTS messages. Validate control-message truncation and require exactly one descriptor so malformed state transfers cannot leak resources or leave ambiguous ownership.

Signed-off-by: Park.Jiyeon <jiyeonnn2@icloud.com>
@Park-Jiyeonn
Park-Jiyeonn force-pushed the fix/supervisor-fd-lifecycle branch from 32b4ed7 to 6d5836d Compare July 23, 2026 07:25

@Zephyrcf Zephyrcf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@Park-Jiyeonn

Copy link
Copy Markdown
Contributor Author

@Zephyrcf If there are no further concerns, could this be merged when convenient?

@imeoer

imeoer commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR! Maybe there is a race between load() and save()/closeFD(), after load() releases the lock, the returned FD may be closed or reused before send() uses it, causing EBADF or sending an unrelated FD, we can consider that dup the FD while holding the lock and close the duplicate after sending.

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.

3 participants