[d8] bounds-check before getting Shell::Worker internal field

src/d8.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 
 // Defined when linking against shared lib on Windows.
 #if defined(USING_V8_SHARED) && !defined(V8_SHARED)
 #define V8_SHARED
 #endif
 
@@ skipping 699 matching lines @@
     }
     worker->StartExecuteInThread(isolate, *script);
   }
 }
 
 
 void Shell::WorkerPostMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
   Local<Context> context = isolate->GetCurrentContext();
+  Local<Value> this_value;
 
   if (args.Length() < 1) {
     Throw(isolate, "Invalid argument");
     return;
   }
 
-  Local<Value> this_value = args.This()->GetInternalField(0);
-  if (!this_value->IsExternal()) {
+  // Bounds-check to avoid fatal error in debug mode

[adamk, 2015/07/07 18:48:26]

I'd just remove this comment: it's not a "bounds-check" in the traditional
sense, it's actually checking for the existence of the internal field.

[caitp (gmail), 2015/07/07 19:00:17]

Acknowledged.

+  if (args.This()->InternalFieldCount() > 0) {
+    this_value = args.This()->GetInternalField(0);
+  }
+  if (this_value.IsEmpty() || !this_value->IsExternal()) {

[adamk, 2015/07/07 18:48:26]

Can the second part of this conditional ever fail? I'm guessing not...

[caitp (gmail), 2015/07/07 19:00:17]

right now, I don't think so

     Throw(isolate, "this is not a Worker");
     return;
   }
 
   Worker* worker =
       static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
 
   Handle<Value> message = args[0];
   ObjectList to_transfer;
   if (args.Length() >= 2) {
@@ skipping 25 matching lines @@
     worker->PostMessage(data);
   } else {
     delete data;
   }
 }
 
 
 void Shell::WorkerGetMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
-
-  Local<Value> this_value = args.This()->GetInternalField(0);
-  if (!this_value->IsExternal()) {
+  Local<Value> this_value;
+  // Bounds-check to avoid fatal error in debug mode

[adamk, 2015/07/07 18:48:26]

Ditto, comment seems off.

+  if (args.This()->InternalFieldCount() > 0) {
+    this_value = args.This()->GetInternalField(0);
+  }
+  if (this_value.IsEmpty() || !this_value->IsExternal()) {

[adamk, 2015/07/07 18:48:26]

Ditto, I don't think IsExternal can fail now

     Throw(isolate, "this is not a Worker");
     return;
   }
 
   Worker* worker =
       static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
 
   SerializationData* data = worker->GetMessage();
   if (data) {
     int offset = 0;
     Local<Value> data_value;
     if (Shell::DeserializeValue(isolate, *data, &offset).ToLocal(&data_value)) {
       args.GetReturnValue().Set(data_value);
     }
     delete data;
   }
 }
 
 
 void Shell::WorkerTerminate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
-  Local<Value> this_value = args.This()->GetInternalField(0);
-  if (!this_value->IsExternal()) {
+  Local<Value> this_value;
+  // Bounds-check to avoid fatal error in debug mode

[adamk, 2015/07/07 18:48:26]

And here...

+  if (args.This()->InternalFieldCount() > 0) {
+    this_value = args.This()->GetInternalField(0);
+  }
+  if (this_value.IsEmpty() || !this_value->IsExternal()) {

[adamk, 2015/07/07 18:48:26]

And here

     Throw(isolate, "this is not a Worker");
     return;
   }
 
   Worker* worker =
       static_cast<Worker*>(Local<External>::Cast(this_value)->Value());
   worker->Terminate();
 }
 #endif  // !V8_SHARED
 
@@ skipping 1620 matching lines @@
 }
 
 }  // namespace v8
 
 
 #ifndef GOOGLE3
 int main(int argc, char* argv[]) {
   return v8::Shell::Main(argc, argv);
 }
 #endif