WIP: src: copy spawn_sync array as strings only #9824
Conversation
This commit changes the cloning mechanism in CopyJsStringArray(). Previously, the array would be cloned, including any problematic setters. This commit creates a new array and uses its setter.
nodejs-github-bot
added
the
c++
mscdex
added
child_process
|
|
||
| // Convert all array elements to string. Modify the js object itself if | ||
| // needed - it's okay since we cloned the original object. | ||
| for (uint32_t i = 0; i < length; i++) { | ||
| if (!js_array->Get(i)->IsString()) | ||
| js_array->Set(i, js_array->Get(i)->ToString(env()->isolate())); | ||
| auto item = input_array->Get(i)->ToString(env()->isolate()); |
There was a problem hiding this comment.
may not be a bad idea to check the Get and ToString (ToLocalChecked is ugly but may be a good thing to have if you don't want to throw exceptions back to js)
|
I have misgivings about fixing this on an ad hoc basis. If we feel that bad magic methods (toString, Symbol.toPrimitive, etc.) should be handled graciously, then we should come up with an overarching strategy and a set of rules to follow, and apply that consistently throughout the code base. We've been inconsistent in the past with how we coerced numbers and it's been painful to change. Let's not repeat that mistake. |
|
@tniessen I'm happy to reopen this, but you need to convince @bnoordhuis I think. |
|
@bnoordhuis Any objections to fixing this ad hoc? I assumed your comment on that topic was pro making such changes. |
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
src
Description of change
This commit changes the cloning mechanism in
CopyJsStringArray(). Previously, the array would be cloned, including any problematic setters. This commit creates a new array and uses its setter.This still needs tests if we decide to go this route, but fixes #9821