invalid floating point uid or gid for spawn/execSync causes uv to assert and abort node

[sam-github]

• Version: 0.12 to v8.0.0-pre

> child_process.spawnSync("cat", {uid: 3.5})
node: ../deps/uv/src/unix/core.c:166: uv_close: Assertion `0' failed.
zsh: abort (core dumped)  ./node

Also

> child_process.execSync("date", {uid: 3.5})
node: ../deps/uv/src/unix/core.c:161: uv_close: Assertion `0' failed.
zsh: abort (core dumped)
% ./node --version
v8.0.0-pre

EDIT: git aborts, too

[sam-github]

See #8312 (comment), /to @saghul not sure if this is a uv bug, or node not checking for bad input before calling uv.

[cjihrig]

I'll take care of the validation in Node in #8312. It might be good for libuv to return EINVAL or something though.

[sam-github]

@cjihrig I assumed you would validate, I reported because #8321 is semver-major, and this bug exists all the way back to 0.12, so across every LTS release, so some sub-set of #8321 will need to be backported.

[sam-github]

@cjihrig did I do right to label it with the node versions it effects? Is that the trigger for the fix to be backported?

[cjihrig]

I think so, but I think backports are more driven by the labels on the PRs (lts-watch-*, dont-land-on-*).

It would great if what each label meant could be documented somewhere (preferably by GitHub). They are usually self explanatory, but we're up to 100 of them.

[sam-github]

@thealphanerd or @nodejs/lts - I think the LTS process needs some docs. I may have over or under labelled, not sure!

[MylesBorins]

@sam-github the watch labels are for prs that need to be audited / backported. I'll work on something this week for the collaborator guide

[sam-github]

@thealphanerd ok, took the watch labels off, just left the version labels on, those are the versions the bug is in

[bnoordhuis]

I don't think #8312 (comment) is a libuv bug. It looks like node.js is trying to close an uninitialized handle, presumably a uv_process_t handle.

[jasnell]

Pulling the v0.12 labels off this given that support for v0.12 is coming to an end in two days and there will not be any further updates to v0.12

[targos]

This was fixed in #8312 but it is semver-major.

[davisjam]

For what it's worth, the opts object is picky about other fields as well. For example, when I supply a timeout as a String instead of a Number I also encounter this crash.

#8312 also fixes the timeout String vs. Number problem.

[TimothyGu]

Is there anything more we can do about this? The PR did land on master already but I don't think it can be backported due to semver-major.

[Trott]

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.