Repository navigation
Fix resource leaks in provisioners and commands - #38585
Conversation
Close file handles, HTTP response bodies, and pipe ends that were previously leaked on certain code paths: - workspace_new.go: Close state file opened via os.Open - file/resource_provisioner.go: Close temp file handle before returning the file path to caller - login.go: Move defer resp.Body.Close() before ioutil.ReadAll so the body is closed even when ReadAll fails - local-exec/resource_provisioner.go: Close the read end of os.Pipe after the copy goroutine completes Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
SarahFrench
left a comment
SarahFrench
left a comment
There was a problem hiding this comment.
Hi @SebTardif thanks for your contribution. I've confirmed that the changes you have made either have test coverage or aren't easily able to be covered by tests.
Before I approve could you please remove the change file from your PR. As your GitHub issue noted, this in an internal code issue and doesn't have user-facing impact.
| pr.Close() | ||
|
|
There was a problem hiding this comment.
Confirmed this has test coverage, e.g. in TestResourceProvider_Apply
| if _, err = file.WriteString(content.AsString()); err != nil { | ||
| file.Close() | ||
| return "", true, err | ||
| } | ||
|
|
||
| file.Close() | ||
| return file.Name(), true, nil |
There was a problem hiding this comment.
These don't have test coverage but that's ok for this PR; testing the file provisioner end to end would require a mock SSH server which we should not implement in this PR.
| defer resp.Body.Close() | ||
|
|
||
| body, err := ioutil.ReadAll(resp.Body) | ||
| if err != nil { | ||
| c.logMOTDError(err) | ||
| c.outputDefaultTFCLoginSuccess() | ||
| return 0 | ||
| } | ||
|
|
||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Minimal change, and has test coverage in TestLogin
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
Has test coverage in TestWorkspace_createWithState
Internal code fix with no user-facing impact. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
SarahFrench
added
the
no-changelog-needed
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
2 participants
Closes #38575
Fix four resource leaks where file handles, HTTP response bodies, or pipe
file descriptors were not properly closed on all code paths.
internal/command/workspace_new.go-- The file opened viaos.Openforreading existing state (when using
terraform workspace new -state=FILE)was never closed. Added
defer f.Close().internal/builtin/provisioners/file/resource_provisioner.go-- The*os.Filehandle fromioutil.TempFileingetSrc()was never closed.The caller re-opens the file by path, so the original handle must be closed
to avoid leaking one file descriptor per file-provisioner invocation.
Added explicit
file.Close()before both return paths.internal/command/login.go-- Thedefer resp.Body.Close()was placedafter
ioutil.ReadAll(resp.Body), meaning the body leaked if ReadAllreturned an error (early return at line 269). Moved the defer to
immediately after the
resp != nilcheck, matching idiomatic Go patterns.internal/builtin/provisioners/local-exec/resource_provisioner.go--os.Pipe()creates two file descriptors. The write end (pw) wasclosed at line 187, but the read end (
pr) was never closed, leakingone file descriptor per
local-execinvocation. Addedpr.Close()after the copy goroutine completes.
Target Release
1.16.x
Rollback Plan
Changes to Security Controls
No changes to security controls.
AI Disclosure
This fix was developed with AI assistance (Grok by xAI) in a human-in-the-loop workflow. The author identified the bug, directed the investigation, reviewed all code changes, and verified correctness. AI was used to assist with code analysis and drafting the fix.
CHANGELOG entry