780272 - Audit calls to MozillaUnRegisterDebugFD

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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)]

Attachment: flush, unregister and then close.

I pushed it to try:

https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Comment 2

[Justin Lebar (not reading bugmail)]

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)]

(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)]

(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)]

(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)]

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)]

Attachment: Now with MozillaUnRegisterDebugFILE :-)

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: In which I find I cannot forward declare FILE

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7af21dd3f06