pipes: close pipe descriptors on open_pipe() error paths - #3134
Conversation
open_pipe() creates a pipe pair with pipe(2) and then returns -1 from three places without closing either end: - cr_fchown() fails while restoring pipe ownership - restore_pipe_data() fails - send_desc_to_peer() fails while handing a descriptor to a peer A fourth descriptor is leaked after the reopen step. Once one end has been closed and tmp holds the surviving descriptor, a failing rst_file_params() returns without closing it. Closing tmp there is safe. reopen_pipe() always closes the descriptor it is given and returns a new one, and the inherited-fd path sets pi->reopen before jumping to the reopen label, so every path that reaches rst_file_params() owns tmp. Add a common err_close label that closes both ends of the pipe, and close tmp when rst_file_params() fails. Test tmp for failure before using it, which makes the trailing tmp >= 0 guard redundant. Signed-off-by: Utkal Singh <singhutkal015@gmail.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, localized, and correctly closes all identified leaked descriptors without altering the success path behavior.
Pull request overview
This PR fixes a file-descriptor leak in CRIU’s pipe restore path by ensuring both ends of a newly created pipe are closed on early error returns in open_pipe(), and by closing the remaining end when rst_file_params() fails.
Changes:
- Replace three
return -1error exits afterpipe(pfd)with a sharederr_closelabel that closespfd[0]andpfd[1]. - Reorder the
tmp < 0check ahead ofrst_file_params()and remove the now-redundanttmp >= 0guard. - Close
tmpwhenrst_file_params()fails to prevent leaking the surviving pipe end.
File summaries
| File | Description |
|---|---|
| criu/pipes.c | Adds a common error-cleanup path to close pipe FDs and plugs a leak when rst_file_params() fails. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## criu-dev #3134 +/- ##
============================================
- Coverage 57.72% 57.70% -0.02%
============================================
Files 161 161
Lines 43920 43930 +10
Branches 9637 9635 -2
============================================
- Hits 25351 25350 -1
- Misses 18330 18341 +11
Partials 239 239 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #3133
open_pipe()creates a pipe pair and returns-1from four places without closing the descriptors: whencr_fchown()fails, whenrestore_pipe_data()fails, whensend_desc_to_peer()fails, and whenrst_file_params()fails after one end has already been closed.This adds a common
err_closelabel that closes both ends, and closestmpwhenrst_file_params()fails.Closing
tmpis safe becausereopen_pipe()always closes the descriptor it is given and returns a new one, and the inherited-fd path setspi->reopenbefore jumping to thereopenlabel, so every path reachingrst_file_params()ownstmp.The
tmp < 0test is moved ahead ofrst_file_params(), which makes the trailingtmp >= 0guard redundant. Behaviour is otherwise unchanged.Testing
make -j8builds clean with no new warnings.gcc -fanalyzer. Note that the analyzer still reports a leak at the function exit after this change: it cannot resolve the computed index inpfd[!(pi->pe->flags & O_WRONLY)]and so cannot tell that the success path transfers the descriptor to the caller. That report is a false positive and is present both before and after this patch, so the analyzer does not confirm the fix; the error paths above were verified by inspection.I was not able to exercise the failing paths end to end locally, as they need a restore to fail at exactly these points. Happy to add a test or take a different approach if you prefer.