Add signature to SET_PROTOTYPE_METHOD and use args.Holder() in methods.

[cscott]

This prevents segfaults when a native method is reassigned to a different
object to corrupt args.This().

Closes #6690.

[Nodejs-Jenkins]

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit cscott/node@37f8296 has the following error(s):

• First line of commit message must be no longer than 50 characters
• Commit message must indicate the subsystem this commit changes
• Commit message line too long: 2

The following commiters were not found in the CLA:

• C. Scott Ananian

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

[indutny]

I think this may have serious impact on performance of crossing C++/JS boundary... cc @trevnorris

[cscott]

It may (or may not) have a performance impact, but it has serious security
implications. See #6690.

[indutny]

@cscott you should never directly expose C++ methods to JS if you care about security, they should always be wrapped in a javascript objects. And another note, if you give access to C++ stuff to the untrusted code - application is already vulnerable.

[bnoordhuis]

I think this may have serious impact on performance of crossing C++/JS boundary

It generally does but this looks okay. Upstream V8 won't inline API function calls that have non-trivial signatures but here the only restriction is on the receiver.

[trevnorris]

@bnoordhuis Is there going to be much performance difference between the following two snippets?

template<typename T>
Local<Object> FunctionCallbackInfo<T>::This() const {
  return Local<Object>(reinterpret_cast<Object*>(values_ + 1));
}


template<typename T>
Local<Object> FunctionCallbackInfo<T>::Holder() const {
  return Local<Object>(reinterpret_cast<Object*>(
      &implicit_args_[kHolderIndex]));
}

[trevnorris]

style: forgot to remove a space before the \. makes the line exceed 80 chars.

[trevnorris]

@indutny If this is added, can we add a custom cpplint check like "\<Unwrap<\w*>(args\.This())" to make sure any future edits don't accidentally use This()?

[bnoordhuis]

Is there going to be much performance difference between the following two snippets?

There should be no measurable difference, they're both bitcasts of a stack slot.

That's assuming they're in the same cache line but even if that isn't the case, I'd be pretty amazed if you could make that show up during profiling.

[trevnorris]

Well, I haven't been able to find any noticeable performance impact. So I'm fine with this.

THB I only half get how v8::Signature works, but i'm also only running on half brain cells.

[indutny]

@trevnorris about linter: go for it :)

[cscott]

Rebased on current HEAD; fixed style issues flagged by @trevnorris above.

[cscott]

The test failures appear to be due to #7309, not this patch.

[cscott]

Rebased on current master.

Note that Buffer uses NODE_SET_METHOD instead of NODE_SET_PROTOTYPE_METHOD and so is not protected by this patch. For example:

b = new Buffer(8);
o = { fill: b.fill };
o.fill(0); // segfaults

[trevnorris]

@cscott True. The reason for that is NODE_SET_PROTOTYPE_METHOD only works for FunctionTemplate, but we're adding methods to a JS Function. I'm not aware of a way to work around this.

[trevnorris]

Merged in 08a5b44.

[cscott]

Thanks!