Skip to content

[POSIX] Clean up temp files in error cases#1020

Open
mhutchinson wants to merge 1 commit into
transparency-dev:mainfrom
mhutchinson:fix/posix-potential-leaks
Open

[POSIX] Clean up temp files in error cases#1020
mhutchinson wants to merge 1 commit into
transparency-dev:mainfrom
mhutchinson:fix/posix-potential-leaks

Conversation

@mhutchinson

Copy link
Copy Markdown
Contributor

No description provided.

@mhutchinson mhutchinson requested a review from a team as a code owner June 25, 2026 12:06
@mhutchinson mhutchinson requested review from AlCutter and phbnf June 25, 2026 12:06
@mhutchinson mhutchinson force-pushed the fix/posix-potential-leaks branch from 0201c60 to 0dbf118 Compare June 25, 2026 12:07
Comment thread storage/posix/file_ops.go
Comment on lines +198 to 215
tmpName := name
success := false
defer func() {
if !success {
if err := os.Remove(tmpName); err != nil {
slog.WarnContext(context.Background(), "Failed to remove temporary file", slog.String("tmpname", tmpName), slog.Any("error", err))
}
name = ""
}
}()
defer func() {
if errC := f.Close(); errC != nil && err == nil {
err = errC
if errC := f.Close(); errC != nil {
if err == nil {
err = errC
}
success = false
}
}()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sorta looks like it's going to defeat the point of the createTemp function by deleting the file it just created?

Note that the comment on the func says it's on the caller to remove the temporary file once they've finished with it.

Maybe you could have createTemp return a "cleanup" func which the caller can use?

Comment thread storage/posix/file_ops.go
if err != nil {
return fmt.Errorf("failed to create temp file: %w", err)
}
success := false

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this would be cleaner if you named the error in the func signature and just check for err != nil in the deferred func - that's a pattern I've seen used for this sort of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants