Closed Bug 1438106 Opened 2 years ago Closed 2 years ago

WriteExtraForMinidump: Resource leak on fd

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1429502])

Attachments

(1 obsolete file)

No description provided.
Comment on attachment 8950853 [details]
Bug 1438106 - Fix a resource leak on fd

https://reviewboard.mozilla.org/r/220088/#review226036

::: toolkit/crashreporter/nsExceptionHandler.cpp:3134
(Diff revision 1)
>      if (fd) {
>        AnnotationTable exceptionTimeAnnotations;
>        ReadAndValidateExceptionTimeAnnotations(fd, exceptionTimeAnnotations);
>        PR_Close(prFd);
>        if (!AppendExtraData(extra, exceptionTimeAnnotations)) {
> +        fclose(fd);

You should be able to put this next to the `PR_Close(prFd)`.

Did I misunderstand the Windows semantics of `fdopen(_open_osfhandle(handle))`? I thought you only needed to close the original handle, but I guess based on this PR you need to close both the handle and the fd.
> You should be able to put this next to the `PR_Close(prFd)`.

I think Coverity thinks that you should always call fclose() even if fopen failed.
Here, if I move it, we will have twice fclose(fd) (which might not be an issue).
Just let me know what you prefer!
Hmmm, I'm not sure I follow; are we sure that we need to close both the HANDLE and the fd?

You've highlighted that in cases where fdopen fails we don't close the HANDLE though, same with cases where _open_osfhandle fails. I'm not sure if either of these are possible.
Closing:
<Alex_Gaynor> I'm looking at our other callers of that function to see what we do
<Alex_Gaynor> yeah, it looks like you only need to close one of them
<Sylvestre> so, false positive
<Alex_Gaynor> I think so
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
Attachment #8950853 - Attachment is obsolete: true
Attachment #8950853 - Flags: review?(agaynor)
You need to log in before you can comment on or make changes to this bug.