fix: utilityProcess.postMessage crash with invalid transferrable

[codebytere]

Description of Change

Refs #37585

Fixes a potential crash in utilityProcess.postMessage when calling with an invalid transferable. From the original PR:

When passing an (unsupported) ArrayBuffer, for example, this happened because we try to unwrap everything in the transferrables list and upstream's WrapperInfo logic doesn't take into account that an object might have the correct number of internal fields but not be a WrappableBase.

This applies here as well.

This PR also removes a check in message_port.cc for at least one argument being passed. The type of message is any per spec, and thus null or undefined is acceptable. As it was, if null or undefined were passed the error wouldn't be thrown regardless, as those values can be converted to v8::Value.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixes a potential crash in utilityProcess.postMessage when calling with an invalid transferable.

[nornagon]

I still don't see how this differs from gin's impl...

here's gin:

void* FromV8Impl(v8::Isolate* isolate, v8::Local<v8::Value> val,
                 WrapperInfo* wrapper_info) {
  if (!val->IsObject())
    return NULL;
  v8::Local<v8::Object> obj = v8::Local<v8::Object>::Cast(val);
  WrapperInfo* info = WrapperInfo::From(obj);

  // If this fails, the object is not managed by Gin. It is either a normal JS
  // object that's not wrapping any external C++ object, or it is wrapping some
  // C++ object, but that object isn't managed by Gin (maybe Blink).
  if (!info)
    return NULL;

  // If this fails, the object is managed by Gin, but it's not wrapping an
  // instance of the C++ class associated with wrapper_info.
  if (info != wrapper_info)
    return NULL;

  return obj->GetAlignedPointerFromInternalField(kEncodedValueIndex);
}

// ...

WrapperInfo* WrapperInfo::From(v8::Local<v8::Object> object) {
  if (object->InternalFieldCount() != kNumberOfInternalFields)
    return NULL;
  WrapperInfo* info = static_cast<WrapperInfo*>(
      object->GetAlignedPointerFromInternalField(kWrapperInfoIndex));
  return info->embedder == kEmbedderNativeGin ? info : NULL;
}

1. Both check if the value is an Object,
2. Both check that object->InternalFieldCount() == kNumberOfInternalFields,
3. Both fetch the kWrapperInfoIndex'th internal field and cast it to WrapperInfo*
4. Our impl checks that this value isn't NULL, where gin's doesn't. I suppose this is the one difference?
5. Both check that info->embedder == kEmbedderNativeGin,
6. gin checks that the info is the right one for the class being unwrapped, where ours doesn't. This seems like a bug in ours and we should check this.

Is (4) the only difference? If so it seems reasonable to upstream a change that adds a null check there. If that's what you mean when you say that's already been tried and rejected, is there a crrev link?

[codebytere]

@nornagon yes and 4 is what causes the crash. Here's the CL: https://chromium-review.googlesource.com/c/chromium/src/+/4335426

[nornagon]

Hm, that's a bit confusing—Camillo said there that this check is unsound. That is, he's saying that an ArrayBuffer should never get to this point, and that's what the embedder should be checking. Adding this check in our helper functions exposes us to the same unsound behavior, which is that info, even if it's not null, is not even guaranteed to be a pointer! Meaning that info && info->embedder could segfault, or return other random data. We haven't really handled this edge case, we've just essentially applied the patch that was rejected upstream that has the same bug :)

I think there is a way around this, which is to reorder (6) and (5). That is, instead of what gin currently does, which is:

1. Get info out of the internal fields
2. Check that info->embedder == kEmbedderNativeGin
3. Check that info == wrapper_info

Instead, do (3) first and then (2). Or even, I think, if info == wrapper_info then we don't even need to check info->embedder really, since the pointer matches the one from the class we're good to go.

I think this would address Camillo's concern?