Repository navigation
spawnSync segfaults when given throwing toString #9820
Description
Activity
This is probably a larger problem and we may need checks for other APIs.
I think we should error but avoid segfaulting.
Reacted by Deian StefanI discussed this with @addaleax on IRC a while ago. Basically we should provide a way for
Utf8ValueandTwoByteValueto signify an error condition. A few options are possible:- return an invalidated
MaybeStackBufferifToStringerrors:void func(Local<Value> value) { Utf8Value str(env->isolate(), value); if (str.IsInvalidated()) return; // do things with str }
- 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
BufferValueuses), 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* successdefault tonullptr. 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?
- return an invalidated
@TimothyGu I agree, Option 1 sounds better to me. We could also use that to see which
Utf8Valuecalls could be replaced byBufferValue, 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 With #11952 closed, should we make a note here of the solution now envisioned?
@Trott Sure. Right now
Utf8ValueandTwoByteValueboth take anyv8::Value, which forces the constructors to convert them tov8::Stringfirst. This conversion step is the only place that can cause failures in these two constructors (think of atoStringhandler that throws). Thus I believe a better way to address this problem would be to make the constructors takev8::String, and forces the user of those classes to handle type conversions and possible errors, instead of relying onIsInvalidated()and other similar methods from theUtf8ValueandTwoByteValueclasses.Reacted by Tobias Nießenfwiw, this fails in v10.x too (as many attempts to fix this seem to have stalled)
This code sample no longer segfaults as of
v10.10.0, and segfaults as late asv10.9.0.Ok, sounds like we should just close it out then.
spawnSync will segfault if called with an object that defines a throwing
toString.Here is a snippet using the high-level
child_processAPI:It may be safer to call toString in JS land before calling into the binding
code.