Closed Bug 1276386 Opened 3 years ago Closed 3 years ago

Prevent Subprocess processes from inheriting extra file descriptors on Windows

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(1 file)

Processes currently inherit all of the parent's inheritable file descriptors, while they only need to inherit standard input, standard output, and standard error.
See Also: → 1276388
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?
Attachment #8757527 - Flags: review?(mhowell)
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.
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 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/
Attachment #8757527 - Flags: review?(mhowell)
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!
Attachment #8757527 - Flags: review?(mhowell) → review+
https://hg.mozilla.org/integration/fx-team/rev/03d6c612af0ddbf22f4fbe12ad034831be38fab7
Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r=mhowell
https://hg.mozilla.org/mozilla-central/rev/03d6c612af0d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.