As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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)
:
: Nathan Froyd [:froydnj]
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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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.