Fix file descriptor leaks in remote import, save, and checkpoint operations#28741
Fix file descriptor leaks in remote import, save, and checkpoint operations#28741SebTardif wants to merge 1 commit into
Conversation
| if i != "" { | ||
| params.Set("import", "true") | ||
| r, err = os.Open(i) | ||
| importFile, err := os.Open(i) |
There was a problem hiding this comment.
idont understand why you are doing this, and then renaming later?
There was a problem hiding this comment.
The intermediate variable is needed because r is declared as io.Reader (no Close method), so we need a typed *os.File to defer close on. Shortened the name from importFile to f to keep it idiomatic.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
samsies, wouldnt a single defer be better?
There was a problem hiding this comment.
This is already a single defer. No changes needed here.
|
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 |
|
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. |
…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 podman-container-tools#28723 and podman-container-tools#28724. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
SebTardif
force-pushed
the
fix-remote-fd-leaks
branch
from
49ff8c5 to
eece6ca
Compare
|
All three inline comments addressed, pushed in eece6ca:
The CI |
2 participants
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.