Fix file descriptor leaks in remote import, save, and checkpoint operations#28741
Open
SebTardif wants to merge 1 commit into
Open
Fix file descriptor leaks in remote import, save, and checkpoint operations#28741SebTardif wants to merge 1 commit into
SebTardif wants to merge 1 commit into
Conversation
…ations Fix four file descriptor leaks: 1. tunnel/images.go Import: os.Open(opts.Source) never closed 2. tunnel/images.go Save: second os.Open for oci-dir/docker-dir never closed 3. bindings/checkpoint.go Restore: os.Open(importPath) never closed 4. container_internal_common.go: os.Create in checkpoint volume export loop not closed on five error paths These are the same class of bug fixed in containers#28723 and containers#28724. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
baude
reviewed
May 20, 2026
|
|
||
| volume, err := c.runtime.GetVolume(v.Name) | ||
| if err != nil { | ||
| volumeTarFile.Close() |
Member
There was a problem hiding this comment.
why shouldnt this be a single deferred close ?
baude
reviewed
May 20, 2026
| if i != "" { | ||
| params.Set("import", "true") | ||
| r, err = os.Open(i) | ||
| importFile, err := os.Open(i) |
Member
There was a problem hiding this comment.
idont understand why you are doing this, and then renaming later?
baude
reviewed
May 20, 2026
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer f.Close() |
Member
There was a problem hiding this comment.
samsies, wouldnt a single defer be better?
Member
|
are you using AI to find these and generate your code and commit messages? remove the origin bits please ... and ptal at https://github.com/containers/podman/blob/main/LLM_POLICY.md |
baude
requested changes
May 20, 2026
Contributor
Author
|
Removed the origin table, apologies for the noise. I did use AI as a tool to help spot these, but I reviewed every fix myself and can speak to each one. I've read the LLM policy. Thanks for pointing it out. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes four file descriptor leaks across remote operations and checkpoint:
tunnel/images.goImport:os.Open(opts.Source)opens the import source file but never closes it. Addeddefer f.Close().tunnel/images.goSave: Foroci-dir/docker-dirformats, a secondos.Open(f.Name())reopens the temp file but the handle is never closed. Addeddefer f.Close().bindings/checkpoint.goRestore:os.Open(i)opens the checkpoint archive, but the result is assigned to anio.Reader, hiding the*os.File. Introduced a typed variable withdefer Close().container_internal_common.goexportCheckpoint: Inside aforloop,os.Create()opens a volume tar file. The explicit close at the end is only reached on the happy path; five error returns skip it. Added explicitClose()on each error path.Same class of bug as #28723 and #28724.
How was this tested?
All fixes are minimal close/defer additions with no behavioral change.
go vetandgo buildpass on all affected packages. The affected remote packages have no existing test files. RequestingNo New Testslabel.Does this PR introduce a user-facing change?
No.