src: don't set non-primitive values on templates

[bnoordhuis]

V8 is going to disallow non-primitive values on v8::FunctionTemplate
and v8::ObjectTemplate because those can't be shared across contexts.
Hide such properties behind a getter.

Fixes: #6216

R=@nodejs/v8 @jeisinger

CI: https://ci.nodejs.org/job/node-test-pull-request/2279/

[bnoordhuis]

I will say I don't really understand the point of this exercise. I can see why non-primitive values would be disallowed but I don't get why it's okay to side-step the issue through a getter.

[trevnorris]

Spoke with @jeisinger recently who pointed me to Object::Clone(). Which does a shallow copy of the object, and is very fast to boot. Would it be instead possible to manually construct a new instance on startup, clone it, then set any different properties if needed. The operation also clones internal aligned indexed array fields. So should be able to use it with constructors like TCPWrap.

[indutny]

Feels really awkward, I don't understand why this is needed. However, it would be simpler to understand if the fix wouldn't be using getters. It looks like we are really trading one incorrect approach for another.

@jeisinger may I ask you to weigh in?

[jeisinger]

this doesn't really fix the issue.

Sorry if my initial description of the problem and the solution wasn't clear. I assume that the security aspect of cross-context leaks aren't much of a concern to node as node doesn't use contexts to separate different scripts that don't trust each other. The main concern here is memory leaks.

Templates are per-isolate objects, while non-primitive values are per context objects. By directly (or indirectly) putting a per-context value on a per-isolate value, you essentially disable GC for all objects in that context (as every non-primitive value keeps its context alive which in turn keeps any object in that context alive).

You might expect that instantiating a template creates a copy of all the values, but it does not, it just reuse the original value you've put in (as it's generally not possible to clean objects - think internal fields: the VM doesn't know whether it's safe to duplicate C pointers).

here, you already have a template for fn, you should can just recv->Set(fn_name, t);

when recv is instantiated, a function in the target context will be created using the t template.

[jeisinger]

I hope my comments made the issue a bit clearer. Besides the leak there's also an correctness issue: If one context e.g. modifies the "methods" array, it'll be modified in all contexts.

The security issue is that you can get to the Array's constructor which is a function and from there to the function constructor which allows for executing arbitrary scripts in the context that the Array was created in.

Please let me know if you have further questions.

[bnoordhuis]

Okay, that makes more sense. Updated, PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/2280/

[jeisinger]

I don't have enough inside in the http module to assess whether the js changes are good, but the C++ parts LGTM

[bnoordhuis]

Polished it a little. Collaborators, PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/2283/

[trevnorris]

LGTM

[thefourtheye]

Is this a back porting change?

[jeisinger]

I landed this in https://codereview.chromium.org/1839983002/ but then reverted it because it broke our node CI

[bnoordhuis]

It's from the commit drop! add v8 Template::Set check - I'm removing it before landing the PR. It might be nice to turn it into a warning so people know they'll need to update their add-ons.

[jeisinger]

I could make it a DCHECK() first, but I'm not sure whether people run with DCHECKs enabled.

I could also introduce two new methods and deprecate Set() - one that takes a Template and one that takes a Primitive. Would that be preferable?

[bnoordhuis]

Releases don't have DCHECKs enabled so that wouldn't help. The way most add-ons are built, they won't see V8 deprecation warnings today but that's something we can probably fix in node-gyp.

[bnoordhuis]

PS: I filed #6232 for the Makefile changes.

[jeisinger]

so you're saying adding the CHECK() back is good for now?

[bnoordhuis]

If you do, are you going to cherry-pick it to v5.0? I was thinking of floating an out-of-tree patch that prints a warning and a stack trace, then give people the v6 release cycle to upgrade their add-ons.

[jeisinger]

I won't cherry pick it. It will be part of v8 5.2

[bnoordhuis]

That sounds fine. We wouldn't upgrade to that until v7 anyway.

[jasnell]

Just FYI... looks like this broke at least one userland module that was depending on the undocumented process.binding('http_parser').HTTPParser.methods. Given that this is an undocumented private API breakage should be expected... however, there may be some other impacts down the road as well.