v8: fix segmentation faults caused by IntegerValue

[pmq20]

This PR fixes #2721 and adds a test to guard it.

Investigations show that the problem was caused by -O3 optimisations of the compiler. When building a debug version the segmentation fault won't be triggered. Moving the return statement inside the inner scope somehow fixes the malicious optimisation.

[rvagg]

Weird, I'd love to lgtm this but I don't see why it would optimise that into a buggy state. Anyone else? @indutny, @bnoordhuis perhaps?

[evanlucas]

Switch from IntegerValue to Int32Value also fixes the issue.

diff --git a/src/process_wrap.cc b/src/process_wrap.cc
index adf1606..90e061f 100644
--- a/src/process_wrap.cc
+++ b/src/process_wrap.cc
@@ -98,7 +98,7 @@ class ProcessWrap : public HandleWrap {
         options->stdio[i].data.stream = stream;
       } else {
         Local<String> fd_key = env->fd_string();
-        int fd = static_cast<int>(stdio->Get(fd_key)->IntegerValue());
+        int fd = static_cast<int>(stdio->Get(fd_key)->Int32Value());
         options->stdio[i].flags = UV_INHERIT_FD;
         options->stdio[i].data.fd = fd;
       }

[Fishrock123]

This needs to be upstreamed to V8 for us to accept it. One of the two already cc'd people should be abel to help with that. :)

[silverwind]

Both patches fix the segfaults for me. While it should be upstreamed, I'd much prefer if we float this now.

[mscdex]

+1 to floating in the meantime or using @evanlucas's proposed change on our side, especially if we want to get 4.0.0 out today/tomorrow.

[silverwind]

ping @pmq20: care to change it to @evanlucas's suggestion?

[brodycj]

I just issued #2726 with the workaround solution by @evanlucas on the v4.0.0-rc branch. I hope we can get them to fix it in v8 in the near future.

[evanlucas]

This is a bug in child_process, _validateStdio specifically. #2727

[indutny]

I don't like landing things that I do not understand. May I ask you to dump assembly output from this function before and after your patch?

[evanlucas]

@indutny sure, know of the best way to do that on OS X?

In short, _validateStdio was setting the fd value to the stdio object instead of stdio.fd in the event that fd === 0. A number was expected whenever the type is fd

[indutny]

Oh, this is reproducible on mac? I'll do the dumps myself then.

[evanlucas]

Yes

[indutny]

This is a somewhat better fix for this problem: #2730 . This removes the cause of the problem. The problem itself is not yet clear to me, but I hope it fixes the crashes.

[indutny]

Nevermind that PR. @evanlucas already figured it out, I just didn't listen well to him. I think I know what the problem is... investigating!

[indutny]

Please do not land it yet, I'm very close to the resolution. The problem is with the handle zapping, and this PR is somewhat a solution for this, but I want to be sure before we do anything like this.

[indutny]

After some thinking, I think we should land: #2727 , and submit this patch to the v8 team. Please let me know if you need help setting up the environment for this.

I'm against landing it without v8 team review, but overall I think that this is the right solution for the problem.

[indutny]

Also, please cc me on that CL. I will provide all required information to the v8 team.

[bnoordhuis]

I left a comment here with details but in a nutshell, I think this PR is correct in that it fixes an insidious scope bug in the else block of Value::IntegerValue().

[rvagg]

OK folks, we have this, #2727 and #2731 with none resolved. From what I can see from the discussion, both this and #2727 look like candidates for landing and have close enough to approval to go in and #2731 seems to have approval although is apparently not the appropriate fix for the bug, just a nice thing to have.

We have to get v4.0.0 out, the timeline we are working to now has ~12 hours less, could owners of these issues or other collaborators involved who understand the issue please step up and merge what should be merged and close anything that shouldn't be merged. I'll push out an rc.3 if we get this done in enough time to bother.

[indutny]

@rvagg #2727 is almost good to go. If @evanlucas won't have to fix it himself - I'll do it for him.

#2731 is not bad to have, but is not a fix for this problem either.

and this PR is a "no go" in our repo, it should be submitted and landed in V8, #2727 is enough to fix the problem on our side.

[indutny]

@rvagg and we are good now since 3bc7e58 just landed.

[indutny]

I'd still look into landing #2731, though, but this depends on @bnoordhuis availability.

[silverwind]

Looks this PR is obsolete now, Thanks for your work in discovering this thought, @pmq20.

[indutny]

@pmq20 let's work together to submit this to v8 team.

[pmq20]

@indutny @silverwind Sorry for the delayed reply. I work in the +8 time zone and just woke up so there's a lot for me to catch up.

[pmq20]

@indutny OK, I'll write a test to reproduce the bug on the sole v8 land.

[indutny]

@pmq20 take a look at cctest-api. This is something that you might be interested in, also your test should call ->IntegerValue() on Object or any non-number value.

[pmq20]

@indutny Thanks for the hint! Looking into it

[pmq20]

@indutny @bnoordhuis @skomski

I have added a v8 test and submitted a CL, let's continue the discussion there.
https://codereview.chromium.org/1303093004/

[indutny]

Yay, thank you!