[v8-dev] Wrap external pointers more carefully. (issue6119009)

Wed, 12 Jan 2011 09:09:26 -0800 

Reviewers: Lasse Reichstein,

Message:
Lasse,

may you have a look?

Description:
Wrap external pointers more carefully.
On 32-bit platforms any pointer with 0 as LSB can be wrapped into Smi. However, 
on 64-bit
platforms it's currently not the case as x64 Smis must have 0s in lower 32 bit 
word.
Even worse, macroassembler Move instruction will try to fetch integer value from 
Smi
and will shift by 32 bits to the right rendering stored pointer incorrect.

BUG=v8:1037

Please review this at http://codereview.chromium.org/6119009/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
  M include/v8.h
  M src/api.cc
  M test/cctest/test-api.cc


Index: include/v8.h
diff --git a/include/v8.h b/include/v8.h
index 883bfad9458ebc31c511ff3ba93568e2d1eab635..db7e24a4799c5d87e533b2c221e5b75f74ed712a 100644 
--- a/include/v8.h
+++ b/include/v8.h
@@ -3432,9 +3432,14 @@ class Internals {
     return ReadField<uint8_t>(map, kMapInstanceTypeOffset);
   }

+  static inline void* GetExternalPointerFromSmi(internal::Object* smi) {
+    return reinterpret_cast<void*>(
+        reinterpret_cast<intptr_t>(smi) >> kSmiShiftSize);
+  }
+
   static inline void* GetExternalPointer(internal::Object* obj) {
     if (HasSmiTag(obj)) {
-      return obj;
+      return GetExternalPointerFromSmi(obj);
     } else if (GetInstanceType(obj) == kProxyType) {
       return ReadField<void*>(obj, kProxyProxyOffset);
     } else {
Index: src/api.cc
diff --git a/src/api.cc b/src/api.cc
index 110468e2313ec75196b7c7604d6c2612ff09e922..8d89909273491b42c8cb00b6346191945e7553ec 100644 
--- a/src/api.cc
+++ b/src/api.cc
@@ -3266,18 +3266,34 @@ void v8::Object::SetInternalField(int index, v8::Handle<Value> value) { 
 }


+static bool CanBeEncodedAsSmi(void* ptr) {
+  const intptr_t address = reinterpret_cast<intptr_t>(ptr);
+  const intptr_t mask = (intptr_t(1) << i::kSmiTagSize) - 1;
+ return ((address & mask) == 0) && i::Smi::IsValid(address >> i::kSmiTagSize); 
+}
+
+
+static i::Smi* EncodeAsSmi(void* ptr) {
+  ASSERT(CanBeEncodedAsSmi(ptr));
+  i::Smi* result = i::Smi::FromIntptr(
+      reinterpret_cast<intptr_t>(ptr) >> i::kSmiTagSize);
+  ASSERT_EQ(ptr, i::Internals::GetExternalPointerFromSmi(result));
+  return result;
+}
+
+
 void v8::Object::SetPointerInInternalField(int index, void* value) {
   ENTER_V8;
-  i::Object* as_object = reinterpret_cast<i::Object*>(value);
-  if (as_object->IsSmi()) {
-    Utils::OpenHandle(this)->SetInternalField(index, as_object);
-    return;
+  if (CanBeEncodedAsSmi(value)) {
+    Utils::OpenHandle(this)->SetInternalField(index, EncodeAsSmi(value));
+  } else {
+    HandleScope scope;
+    i::Handle<i::Proxy> proxy =
+ i::Factory::NewProxy(reinterpret_cast<i::Address>(value), i::TENURED); 
+    if (!proxy.is_null())
+        Utils::OpenHandle(this)->SetInternalField(index, *proxy);
   }
-  HandleScope scope;
-  i::Handle<i::Proxy> proxy =
- i::Factory::NewProxy(reinterpret_cast<i::Address>(value), i::TENURED); 
-  if (!proxy.is_null())
-      Utils::OpenHandle(this)->SetInternalField(index, *proxy);
+  ASSERT_EQ(value, GetPointerFromInternalField(index));
 }


@@ -3560,11 +3576,13 @@ Local<Value> v8::External::Wrap(void* data) {
   LOG_API("External::Wrap");
   EnsureInitialized("v8::External::Wrap()");
   ENTER_V8;
-  i::Object* as_object = reinterpret_cast<i::Object*>(data);
-  if (as_object->IsSmi()) {
-    return Utils::ToLocal(i::Handle<i::Object>(as_object));
-  }
-  return ExternalNewImpl(data);
+
+  v8::Local<v8::Value> result = CanBeEncodedAsSmi(data)
+      ? Utils::ToLocal(i::Handle<i::Object>(EncodeAsSmi(data)))
+      : v8::Local<v8::Value>(ExternalNewImpl(data));
+
+  ASSERT_EQ(data, Unwrap(result));
+  return result;
 }
@@ -3572,7 +3590,7 @@ void* v8::Object::SlowGetPointerFromInternalField(int index) { 
   i::Handle<i::JSObject> obj = Utils::OpenHandle(this);
   i::Object* value = obj->GetInternalField(index);
   if (value->IsSmi()) {
-    return value;
+    return i::Internals::GetExternalPointerFromSmi(value);
   } else if (value->IsProxy()) {
     return reinterpret_cast<void*>(i::Proxy::cast(value)->proxy());
   } else {
@@ -3586,8 +3604,7 @@ void* v8::External::FullUnwrap(v8::Handle<v8::Value> wrapper) { 
   i::Handle<i::Object> obj = Utils::OpenHandle(*wrapper);
   void* result;
   if (obj->IsSmi()) {
-    // The external value was an aligned pointer.
-    result = *obj;
+    result = i::Internals::GetExternalPointerFromSmi(*obj);
   } else if (obj->IsProxy()) {
     result = ExternalValueImpl(obj);
   } else {
Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 9539973693b10cb0fd5e0e5546c41e762737ba7a..a70eeec493a5480aa607bb3b7494778037cf38cf 100644 
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -814,6 +814,39 @@ THREADED_TEST(FunctionTemplate) {
 }


+static v8::Handle<v8::Value> callback(const v8::Arguments& args) {
+  int* value = reinterpret_cast<int*>(v8::External::Unwrap(args.Data()));
+  CHECK_EQ(42, *value);
+  return v8::Integer::New(*value);
+}
+
+
+THREADED_TEST(PointerAsData) {
+  v8::HandleScope scope;
+  LocalContext env;
+
+  int* value = new int;
+  *value = 42;
+  v8::Handle<v8::Value> data = v8::External::Wrap(value);
+
+  v8::Handle<v8::Object> obj = v8::Object::New();
+  obj->Set(v8_str("func"),
+           v8::FunctionTemplate::New(callback, data)->GetFunction());
+  env->Global()->Set(v8_str("obj"), obj);
+
+  CHECK(CompileRun(
+        "function foo() {\n"
+        "  for (var i = 0; i < 13; i++) {\n"
+        "    if (42 != obj.func()) throw 'oops';\n"
+        "  }\n"
+        "  return true;\n"
+        "}\n"
+        "foo(), true")->BooleanValue());
+
+  delete value;
+}
+
+
 THREADED_TEST(FindInstanceInPrototypeChain) {
   v8::HandleScope scope;
   LocalContext env;


--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to