The default bug view has changed. See this FAQ.

Audit calls to MozillaUnRegisterDebugFD

RESOLVED FIXED in mozilla17

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

fclose can call write, so in some places we have code that looks like

int fd = fileno(foo);
fclose(foo);
MozillaUnRegisterDebugFD(fd);

There is a possible race condition: A new file could be open after fclose and before MozillaUnRegisterDebugFD and it could reused the file descriptor number. If that file is also used for debug, the call MozillaRegisterDebugFD can hit the assert:

        MOZ_ASSERT(std::find(Vec.begin(), Vec.end(), fd) == Vec.end())

It looks like the correct pattern is

fflush(foo);
MozillaUnRegisterDebugFD(fileno(foo));
fclose(foo);
Created attachment 650078 [details] [diff] [review]
flush, unregister and then close.

I pushed it to try:

https://tbpl.mozilla.org/?tree=Try&pusher=respindola@mozilla.com
Attachment #650078 - Flags: review?(justin.lebar+bug)
Can we make UnRegisterDebugFD do the fflush, so as to avoid the footgun of forgetting to flush before un-registering?
(In reply to Justin Lebar [:jlebar] from comment #2)
> Can we make UnRegisterDebugFD do the fflush, so as to avoid the footgun of
> forgetting to flush before un-registering?

It takes a file descriptor since some of the users don't use FILEs. I can add a version that takes a FILE* if you want.
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #3)
> It takes a file descriptor since some of the users don't use FILEs. I can
> add a version that takes a FILE* if you want.

Hmm...you can't take a FILE* and see whether it has anything currently buffered, can you?
(In reply to Justin Lebar [:jlebar] from comment #4)
> (In reply to Rafael Ávila de Espíndola (:espindola) from comment #3)
> > It takes a file descriptor since some of the users don't use FILEs. I can
> > add a version that takes a FILE* if you want.
> 
> Hmm...you can't take a FILE* and see whether it has anything currently
> buffered, can you?

I don't think so, but I think it would be OK to have

void UnregisterDebugFD(FILE *f) {
 fflush(f);
 UnregisterDebugFD(fileno(f));
}
I think that's OK, then.  It's kind of weird to call it UnregisterDebugFD if it takes a FILE, but no big deal.
Created attachment 650535 [details] [diff] [review]
Now with MozillaUnRegisterDebugFILE :-)
Attachment #650078 - Attachment is obsolete: true
Attachment #650078 - Flags: review?(justin.lebar+bug)
Attachment #650535 - Flags: review?(justin.lebar+bug)
Created attachment 650540 [details] [diff] [review]
In which I find I cannot forward declare FILE
Attachment #650535 - Attachment is obsolete: true
Attachment #650535 - Flags: review?(justin.lebar+bug)
Attachment #650540 - Flags: review?(justin.lebar+bug)
Attachment #650540 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/mozilla-central/rev/c7af21dd3f06
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.