Kris Maglione [:kmag] Assignee
	Description • 8 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 1 • 8 years ago

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

	Molly Howell (she/her) [:mhowell]
	Comment 2 • 8 years ago

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?

	Kris Maglione [:kmag] Assignee
	Comment 3 • 8 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 4 • 8 years ago

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.

	Kris Maglione [:kmag] Assignee
	Comment 5 • 8 years ago

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/

	Molly Howell (she/her) [:mhowell]
	Comment 6 • 8 years ago

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!

	Kris Maglione [:kmag] Assignee
	Comment 7 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 8 • 8 years ago
bugherder

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