Skip to content

libct: add a defer fd close in createDeviceNode#5022

Closed
lifubang wants to merge 1 commit intoopencontainers:mainfrom
lifubang:fix-fdclose-in-createDeviceNode
Closed

libct: add a defer fd close in createDeviceNode#5022
lifubang wants to merge 1 commit intoopencontainers:mainfrom
lifubang:fix-fdclose-in-createDeviceNode

Conversation

@lifubang
Copy link
Copy Markdown
Member

@lifubang lifubang commented Nov 18, 2025

Fix: #5021

Without deferring the closure of this file descriptor, starting a container with a very large number of devices can hit the RLIMIT_NOFILE limit.

@lifubang
libct: add a defer fd close in createDeviceNode
4b78298 
Signed-off-by: lifubang <lifubang@acmcoder.com>
@cyphar cyphar mentioned this pull request Nov 18, 2025
Closed
cyphar
cyphar approved these changes Nov 18, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Thanks, my bad for missing this.

@cyphar cyphar added backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 backport/1.3-todo A PR in main branch which needs to be backported to release-1.3 backport/1.4-todo A PR in main branch which needs to backported to release-1.4 easy-to-review labels Nov 18, 2025
@rkolchmeyer
Copy link
Copy Markdown

rkolchmeyer commented Nov 19, 2025

I've applied this change locally to 1.2.8 and it doesn't appear to solve the problem. I will try debugging a bit more.

@lifubang
Copy link
Copy Markdown
Member Author

lifubang commented Nov 19, 2025

I've applied this change locally to 1.2.8 and it doesn't appear to solve the problem.

I think you also need to apply this one: cyphar/filepath-securejoin#85

@rkolchmeyer
Copy link
Copy Markdown

rkolchmeyer commented Nov 19, 2025

Thanks for the pointer - however, adding that patch in doesn't seem to solve our issue either.

One additional data point is that the leak we're seeing appears to be non-deterministic; for example, the issue doesn't reproduce when we run our reproducer under strace. I'll post more details on the issue when I have them.

@rkolchmeyer
Copy link
Copy Markdown

rkolchmeyer commented Nov 19, 2025

I apologize - these patches do solve our problem, my test just wasnt compatible with some runc security features. Thanks for the fix, and sorry for the noise.

@lifubang
Copy link
Copy Markdown
Member Author

lifubang commented Nov 19, 2025

I apologize - these patches do solve our problem, my test just wasnt compatible with some runc security features. Thanks for the fix, and sorry for the noise.

No need to apologize, many thanks for your test and confirm.

@lifubang lifubang mentioned this pull request Nov 20, 2025
Merged
@cyphar
Copy link
Copy Markdown
Member

cyphar commented Nov 20, 2025

This has been cherry-picked into #5026, closing in favour of that.

@cyphar cyphar closed this Nov 20, 2025
This was referenced Nov 20, 2025
Merged
Merged
Merged
@cyphar cyphar removed backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 backport/1.3-todo A PR in main branch which needs to be backported to release-1.3 backport/1.4-todo A PR in main branch which needs to backported to release-1.4 labels Nov 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cyphar cyphar cyphar approved these changes

Assignees

No one assigned

Labels

easy-to-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[1.2.8] runc appears to apply rlimits to itself prior to validating device nodes

3 participants

@lifubang @rkolchmeyer @cyphar