1276386 - Prevent Subprocess processes from inheriting extra file descriptors on Windows

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Kris Maglione [:kmag]]

Processes currently inherit all of the parent's inheritable file descriptors, while they only need to inherit standard input, standard output, and standard error.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r?mhowell

Review commit: https://reviewboard.mozilla.org/r/55964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55964/

Comment 2

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8757527 [details]
MozReview Request: Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r?mhowell

https://reviewboard.mozilla.org/r/55964/#review52680

Are we doing this change because there's a test that fails without it? If not, we should add one.

::: toolkit/modules/subprocess/subprocess_shared_win.js:221
(Diff revision 1)
>      win32.PROCESS_INFORMATION.ptr, /* out lpProcessInformation */
>    ],
>  
> +  DeleteProcThreadAttributeList: [
> +    win32.WINAPI,
> +    win32.BOOL,

This function returns void.

::: toolkit/modules/subprocess/subprocess_shared_win.js:405
(Diff revision 1)
> +    libc.InitializeProcThreadAttributeList;
> +    libc.DeleteProcThreadAttributeList;
> +    libc.UpdateProcThreadAttribute;
> +  } catch (e) {
> +    // This is only supported in Windows Vista and later.
> +    return null;

Silently failing this operation on XP doesn't break anything else?

Comment 3

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/55964/#review52680

> This function returns void.

Oh, interesting. We have it defined as returning BOOL in process_utils_win.cpp.

> Silently failing this operation on XP doesn't break anything else?

It doesn't break anything, the process just inherits all inheritable file descriptors.

Comment 4

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/55964/#review52680

No, there isn't a test that fails. I'm just worried that the inherited file descriptors will cause subtle bugs down the road.

I thought about adding a test, but my WinAPI-fu is not that good. I guess I'll give it a shot.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8757527 [details]
MozReview Request: Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r?mhowell

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55964/diff/1-2/

Comment 6

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8757527 [details]
MozReview Request: Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r?mhowell

https://reviewboard.mozilla.org/r/55964/#review53128

Looks good now, thanks!

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/03d6c612af0ddbf22f4fbe12ad034831be38fab7
Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r=mhowell

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03d6c612af0d