[POSIX] Clean up temp files in error cases#1020
Conversation
mhutchinson
force-pushed
the
fix/posix-potential-leaks
branch
from
0201c60 to
0dbf118
Compare
| 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 | ||
| } | ||
| }() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This only deletes files that would otherwise get leaked when we return an error. I agree it's hard to read though. I think dropping the success variable and using a named error would help. But would it completely mitigate your concern?
e.g. before we tried to create a temp file and write to it. The create worked, but the write didn't work (perhaps threw an error, or the len check didn't work out). We don't give the filename to the user, but before this change the created file stays around. Whatever the problem was on disk is probably now getting worse.
There was a problem hiding this comment.
Gotcha, ok - yeah, I think named returns vars would help, I think you could eliminate tmpName too that way?
| if err != nil { | ||
| return fmt.Errorf("failed to create temp file: %w", err) | ||
| } | ||
| success := false |
There was a problem hiding this comment.
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.
2 participants
No description provided.