Closed
Bug 780272
Opened 12 years ago
Closed 12 years ago
Audit calls to MozillaUnRegisterDebugFD
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: espindola, Assigned: espindola)
Details
Attachments
(1 file, 2 obsolete files)
4.75 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
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);
Assignee | ||
Comment 1•12 years ago
|
||
I pushed it to try: https://tbpl.mozilla.org/?tree=Try&pusher=respindola@mozilla.com
Attachment #650078 -
Flags: review?(justin.lebar+bug)
Comment 2•12 years ago
|
||
Can we make UnRegisterDebugFD do the fflush, so as to avoid the footgun of forgetting to flush before un-registering?
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Comment 4•12 years ago
|
||
(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?
Assignee | ||
Comment 5•12 years ago
|
||
(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)); }
Comment 6•12 years ago
|
||
I think that's OK, then. It's kind of weird to call it UnregisterDebugFD if it takes a FILE, but no big deal.
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #650078 -
Attachment is obsolete: true
Attachment #650078 -
Flags: review?(justin.lebar+bug)
Attachment #650535 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #650535 -
Attachment is obsolete: true
Attachment #650535 -
Flags: review?(justin.lebar+bug)
Attachment #650540 -
Flags: review?(justin.lebar+bug)
Updated•12 years ago
|
Attachment #650540 -
Flags: review?(justin.lebar+bug) → review+
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c7af21dd3f06
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•