spawnSync segfaults when given throwing toString

[deian]

• Version: 6.4.0 - 8.0.0
• Platform:
• Subsystem:

spawnSync will segfault if called with an object that defines a throwing toString.

Here is a snippet using the high-level child_process API:

const spawn = require('child_process').spawnSync;

const args = [];
const obj = {};
obj.toString = () => {
  throw 'yo';
  // causes ToString on spawn_sync.cc:964 to return empty handle; Set getfaults
    
};
args[0] = obj;
spawn('ls', args);

It may be safer to call toString in JS land before calling into the binding
code.

• @mlfbrown for working on this with me.

[Fishrock123]

This is probably a larger problem and we may need checks for other APIs.

I think we should error but avoid segfaulting.

[TimothyGu]

I discussed this with @addaleax on IRC a while ago. Basically we should provide a way for Utf8Value and TwoByteValue to signify an error condition. A few options are possible:

1. return an invalidated MaybeStackBuffer if ToString errors:

   void func(Local<Value> value) {
     Utf8Value str(env->isolate(), value);
     if (str.IsInvalidated())
       return;
     // do things with str
   }

2. provide a constructor that sets a boolean pointer in case of error:

   void func(Local<Value> value) {
     bool success = false;
     Utf8Value str(env->isolate(), value, &success);
     if (!success)
       return;
     // do things with str
   }

Option 1 is the cleaner solution (EDIT: and it is also the one BufferValue uses), but given the fact that these two classes have been used all over the code base it might be difficult to change them all.

Option 2 is more compatible since we can make bool* success default to nullptr. I'm leaning toward this option, but at the same time I suspect it might be better to just do a full audit at once as opposed to delaying it for the future.

Any thoughts on this?

[addaleax]

@TimothyGu I agree, Option 1 sounds better to me. We could also use that to see which Utf8Value calls could be replaced by BufferValue, I would guess there’s quite a few places where that makes sense. (We’d have to split those into different changesets … one is definitely semver-patch and one is definitely semver-minor.)

Do you want to take that on? If not, I should be able to do it too.

[TimothyGu]

@addaleax #11952. Feel free to fill in on the files I haven't yet gone through!

[Trott]

@TimothyGu With #11952 closed, should we make a note here of the solution now envisioned?

[TimothyGu]

@Trott Sure. Right now Utf8Value and TwoByteValue both take any v8::Value, which forces the constructors to convert them to v8::String first. This conversion step is the only place that can cause failures in these two constructors (think of a toString handler that throws). Thus I believe a better way to address this problem would be to make the constructors take v8::String, and forces the user of those classes to handle type conversions and possible errors, instead of relying on IsInvalidated() and other similar methods from the Utf8Value and TwoByteValue classes.

[gireeshpunathil]

fwiw, this fails in v10.x too (as many attempts to fix this seem to have stalled)