Last Comment Bug 780272 - Audit calls to MozillaUnRegisterDebugFD
: Audit calls to MozillaUnRegisterDebugFD
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-03 13:38 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-08-11 19:56 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
flush, unregister and then close. (2.33 KB, patch)
2012-08-08 06:09 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Now with MozillaUnRegisterDebugFILE :-) (4.21 KB, patch)
2012-08-09 06:47 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
In which I find I cannot forward declare FILE (4.75 KB, patch)
2012-08-09 07:08 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-03 13:38:41 PDT
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);
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-08 06:09:37 PDT
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
Comment 2 Justin Lebar (not reading bugmail) 2012-08-08 12:18:16 PDT
Can we make UnRegisterDebugFD do the fflush, so as to avoid the footgun of forgetting to flush before un-registering?
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-08 12:20:17 PDT
(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 Justin Lebar (not reading bugmail) 2012-08-08 12:28:07 PDT
(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?
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-08 12:31:40 PDT
(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 Justin Lebar (not reading bugmail) 2012-08-08 12:37:56 PDT
I think that's OK, then.  It's kind of weird to call it UnregisterDebugFD if it takes a FILE, but no big deal.
Comment 7 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-09 06:47:21 PDT
Created attachment 650535 [details] [diff] [review]
Now with MozillaUnRegisterDebugFILE :-)
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-09 07:08:17 PDT
Created attachment 650540 [details] [diff] [review]
In which I find I cannot forward declare FILE
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-11 19:56:07 PDT
https://hg.mozilla.org/mozilla-central/rev/c7af21dd3f06

Note You need to log in before you can comment on or make changes to this bug.