1556993 - PoisonIOInterposer doesn't handle FILE/fd/handle conversion failures

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

When PoisonIOInterposer is first initialized, it registers stdout and stderr as debug files (not to be monitored) :
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/xpcom/build/PoisonIOInterposerWin.cpp#341-342 :

void InitPoisonIOInterposer() {
   ...
  // Stdout and Stderr are OK.
  MozillaRegisterDebugFD(1);
  MozillaRegisterDebugFD(2);

Notice that there are no checks.
Does this code expect these fds to always be present (and is happy with things going bad if not), or does it not care whether they exist and just wants to register them if they do?

The fd-registration function converts the fd into a local handle and then registers that handle:
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/xpcom/build/PoisonIOInterposerBase.cpp#211 :

void MozillaRegisterDebugFD(int aFd) {
  MozillaRegisterDebugHandle(FileDescriptorToHandle(aFd));
}

No checks again here.

In particular, on Windows
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/xpcom/build/PoisonIOInterposerBase.cpp#24 :

inline intptr_t FileDescriptorToHandle(int aFd) { return _get_osfhandle(aFd); }

If the fd is not valid, or if the stdout/stderr is not associated with an output stream (e.g., in a Windows app without console), _get_osfhandle could return -1 or -2 resp.

Finally, in the handle-registration function, there's an assert that a handle is only registered once:
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/xpcom/build/PoisonIOInterposerBase.cpp#206 :

void MozillaRegisterDebugHandle(intptr_t aHandle) {
  DebugFilesAutoLock lockedScope;
  FdList& DebugFileIDs = getDebugFileIDs();
  MOZ_ASSERT(!DebugFileIDs.Contains(aHandle));
  DebugFileIDs.Add(aHandle);
}

So, from the top:

• We want to register fds 1 and then 2,
• if stdout&err are not present, we get handle -2 for both,
• We add -2 the first time, all good,
• We add -2 again and hit the MOZ_ASSERT.

I've somehow hit that condition in recent work on Windows (related to bug 1492121), and it took me a while to find this issue.
I tried locally to add a simple test&bail in MozillaRegisterDebugFD, and this cleared my problem; or maybe it wallpapered over it? (I'm still investigating why my code was influencing fds 1&2)

In any case, I think failures should be better handled, either to properly ignore them (so we accept that some files may not actually exist and move on), or to better report the issue.
There are other places in that file where return values are not checked for errors.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1556993 - Ignore invalid handles in Mozilla{,Un}RegisterDebugHandle - r?erahm

In particular, on Windows stdin/out/err may not be associated with a stream
(e.g., app without console window), in which case _get_osfhandle() returns -2.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a01cf6b0fc83
Ignore invalid handles in Mozilla{,Un}RegisterDebugHandle - r=erahm

Comment 3

[Oana Pop-Rus]

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