Fix memory errors in read_property.

cscott · cscott · commit 30ca3f9f9495 · 2015-10-21T12:36:17.000-04:00
Use the __get magic method, instead of attempting to implement the
read_property handler, since the expected refcount properties of
the value returned from the read_property handler seem to be exceedingly
baroque.
diff --git a/src/asyncmessageworker.h b/src/asyncmessageworker.h
@@ -63,16 +63,19 @@ namespace node_php_embed {
   }
   // Map PHP object to an index (PHP thread only)
   objid_t IdForPhpObj(zval *z) {
-      if (phpObjToId_.count(z)) {
-          return phpObjToId_.at(z);
+      assert(Z_TYPE_P(z) == IS_OBJECT);
+      zend_object_handle handle = Z_OBJ_HANDLE_P(z);
+      if (phpObjToId_.count(handle)) {
+          return phpObjToId_.at(handle);
       }
       uv_mutex_lock(&id_lock_);
       objid_t id = (nextId_++);
       uv_mutex_unlock(&id_lock_);
-      Z_ADDREF_P(z);
       if (id >= phpObjList_.size()) { phpObjList_.resize(id+1); }
+      // xxx clone/separate z?
+      Z_ADDREF_P(z);
       phpObjList_[id] = z;
-      phpObjToId_[z] = id;
+      phpObjToId_[handle] = id;
       return id;
   }
   // returned value is owned by objectmapper, caller should not release it.
@@ -82,7 +85,7 @@ namespace node_php_embed {
       if (z.IsNull()) {
           node_php_jsobject_create(z.Ptr(), channel, id TSRMLS_CC);
           phpObjList_[id] = z.Ptr();
-          phpObjToId_[z.Ptr()] = id;
+          phpObjToId_[Z_OBJ_HANDLE_P(z.Ptr())] = id;
           // one excess reference: owned by objectmapper
           return z.Escape();
       }
@@ -94,7 +97,7 @@ namespace node_php_embed {
       zval *z = (id < phpObjList_.size()) ? phpObjList_[id] : NULL;
       if (z) {
           phpObjList_[id] = NULL;
-          phpObjToId_.erase(z);
+          phpObjToId_.erase(Z_OBJ_HANDLE_P(z));
           zval_ptr_dtor(&z);
       }
   }
@@ -255,7 +258,7 @@ namespace node_php_embed {
   Nan::Persistent<v8::NativeWeakMap> jsObjToId_;
   // PHP Object mapping
   // Read/writable only from PHP thread
-  std::unordered_map<zval*,objid_t> phpObjToId_;
+  std::unordered_map<zend_object_handle,objid_t> phpObjToId_;
   std::vector<zval*> phpObjList_;
   // Ids are allocated from both threads, so mutex is required
   uv_mutex_t id_lock_;
diff --git a/src/messages.h b/src/messages.h
@@ -151,7 +151,7 @@ class PhpGetPropertyMsg : public MessageToPhp {
         : MessageToPhp(m), obj_(m, obj), name_(m, name) { }
  protected:
     virtual void InPhp(ObjectMapper *m TSRMLS_DC) {
-        ZVal obj(ZEND_FILE_LINE_C), name(ZEND_FILE_LINE_C);
+        ZVal obj{ZEND_FILE_LINE_C}, name{ZEND_FILE_LINE_C};
         zval *r;
         zend_class_entry *ce;
         zend_property_info *property_info;
diff --git a/src/node_php_embed.cc b/src/node_php_embed.cc
@@ -61,7 +61,7 @@ class node_php_embed::PhpRequestWorker : public AsyncMessageWorker {
         }
         NODE_PHP_EMBED_G(messageChannel) = &messageChannel;
         {
-        ZVal retval(ZEND_FILE_LINE_C);
+        ZVal retval{ZEND_FILE_LINE_C};
         zend_first_try {
             char eval_msg[] = { "request" }; // shows up in error messages
             if (FAILURE == zend_eval_string_ex(source_, *retval, eval_msg, true TSRMLS_CC)) {
@@ -113,7 +113,7 @@ static int node_php_embed_ub_write(const char *str, unsigned int str_length TSRM
     AsyncMessageWorker::MessageChannel *messageChannel = NODE_PHP_EMBED_G(messageChannel);
     PhpRequestWorker *worker = (PhpRequestWorker *)
         (messageChannel->GetWorker());
-    ZVal stream(ZEND_FILE_LINE_C);
+    ZVal stream{ZEND_FILE_LINE_C};
     worker->GetStream().ToPhp(messageChannel, stream TSRMLS_CC);
     zval buf; INIT_ZVAL(buf); // stack allocate a null zval as a placeholder
     zval *args[] = { &buf };
@@ -146,10 +146,10 @@ static void node_php_embed_register_server_variables(zval *track_vars_array TSRM
     // relative to the document root."
     // XXX
     // Put PHP-wrapped version of node context object in $_SERVER['CONTEXT']
-    ZVal context(ZEND_FILE_LINE_C);
+    ZVal context{ZEND_FILE_LINE_C};
     worker->GetContext().ToPhp(messageChannel, context TSRMLS_CC);
     char contextName[] = { "CONTEXT" };
-    php_register_variable_ex(contextName, context.Transfer(), track_vars_array TSRMLS_CC);
+    php_register_variable_ex(contextName, context.Transfer(TSRMLS_C), track_vars_array TSRMLS_CC);
     // XXX call a JS function passing in $_SERVER to allow init?
 }
 
diff --git a/src/node_php_jsobject_class.cc b/src/node_php_jsobject_class.cc
@@ -13,6 +13,8 @@ extern "C" {
 #include "values.h"
 #include "macros.h"
 
+#define USE_MAGIC_ISSET 0
+
 using namespace node_php_embed;
 
 /* Class Entries */
@@ -81,6 +83,36 @@ class JsHasPropertyMsg : public MessageToJs {
     }
 };
 
+#if USE_MAGIC_ISSET
+
+PHP_METHOD(JsObject, __isset) {
+    zval *member;
+    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z/", &member) == FAILURE) {
+        zend_throw_exception(
+            zend_exception_get_default(TSRMLS_C),
+            "bad property name for __isset", 0 TSRMLS_CC);
+        return;
+    }
+    convert_to_string(member);
+    node_php_jsobject *obj = (node_php_jsobject *)
+        zend_object_store_get_object(this_ptr TSRMLS_CC);
+    JsHasPropertyMsg msg(obj->channel, obj->id, member, 0);
+    obj->channel->Send(&msg);
+    msg.WaitForResponse();
+    // ok, result is in msg.retval_ or msg.exception_
+    if (msg.HasException()) {
+         zend_throw_exception_ex(
+            zend_exception_get_default(TSRMLS_C), 0 TSRMLS_CC,
+            "JS exception thrown during __isset of \"%*s\"",
+            Z_STRLEN_P(member), Z_STRVAL_P(member));
+        return;
+    }
+    RETURN_BOOL(msg.retval_.AsBool());
+}
+
+#else
+// By overriding has_property we can implement property_exists correctly,
+// and also handle empty arrays.
 static int node_php_jsobject_has_property(zval *object, zval *member, int has_set_exists ZEND_HASH_KEY_DC TSRMLS_DC) {
     /* param has_set_exists:
      * 0 (has) whether property exists and is not NULL  - isset()
@@ -99,6 +131,7 @@ static int node_php_jsobject_has_property(zval *object, zval *member, int has_se
     if (msg.HasException()) { return false; /* sigh */ }
     return msg.retval_.AsBool();
 }
+#endif /* USE_MAGIC_ISSET */
 
 class JsReadPropertyMsg : public MessageToJs {
     Value object_;
@@ -129,23 +162,30 @@ class JsReadPropertyMsg : public MessageToJs {
     }
 };
 
-static zval *node_php_jsobject_read_property(zval *object, zval *member, int type ZEND_HASH_KEY_DC TSRMLS_DC) {
-    ZVal retval(ZEND_FILE_LINE_C);
-    if (Z_TYPE_P(member) != IS_STRING) {
-        return retval.Escape();
+
+PHP_METHOD(JsObject, __get) {
+    zval *member;
+    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z/", &member) == FAILURE) {
+        zend_throw_exception(zend_exception_get_default(TSRMLS_C), "bad property name", 0 TSRMLS_CC);
+        return;
     }
+    convert_to_string(member);
     node_php_jsobject *obj = (node_php_jsobject *)
-        zend_object_store_get_object(object TSRMLS_CC);
-    JsReadPropertyMsg msg(obj->channel, obj->id, member, type);
+        zend_object_store_get_object(this_ptr TSRMLS_CC);
+    JsReadPropertyMsg msg(obj->channel, obj->id, member, 0);
     obj->channel->Send(&msg);
     msg.WaitForResponse();
     // ok, result is in msg.retval_ or msg.exception_
-    if (msg.HasException()) { msg.retval_.SetNull(); /* sigh */ }
-    msg.retval_.ToPhp(obj->channel, retval TSRMLS_CC);
-    return retval.Escape();
+    if (msg.HasException()) {
+         zend_throw_exception_ex(
+            zend_exception_get_default(TSRMLS_C), 0 TSRMLS_CC,
+            "JS exception thrown during __get of \"%*s\"",
+            Z_STRLEN_P(member), Z_STRVAL_P(member));
+        return;
+    }
+    msg.retval_.ToPhp(obj->channel, return_value, return_value_ptr TSRMLS_CC);
 }
 
-
 static void node_php_jsobject_free_storage(void *object, zend_object_handle handle TSRMLS_DC) {
     node_php_jsobject *c = (node_php_jsobject *) object;
 
@@ -224,11 +264,22 @@ STUB_METHOD(__construct)
 STUB_METHOD(__sleep)
 STUB_METHOD(__wakeup)
 
+ZEND_BEGIN_ARG_INFO_EX(node_php_jsobject_one_arg, 0, 0, 1)
+    ZEND_ARG_INFO(0, member)
+ZEND_END_ARG_INFO()
+ZEND_BEGIN_ARG_INFO_EX(node_php_jsobject_one_arg_retref, 0, 1, 1)
+    ZEND_ARG_INFO(0, member)
+ZEND_END_ARG_INFO()
+
 static const zend_function_entry node_php_jsobject_methods[] = {
     PHP_ME(JsObject, __construct, NULL, ZEND_ACC_PUBLIC|ZEND_ACC_CTOR)
     PHP_ME(JsObject, __sleep,     NULL, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
     PHP_ME(JsObject, __wakeup,    NULL, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
-    {NULL, NULL, NULL}
+#if USE_MAGIC_ISSET
+    PHP_ME(JsObject, __isset, node_php_jsobject_one_arg, ZEND_ACC_PUBLIC)
+#endif
+    PHP_ME(JsObject, __get, node_php_jsobject_one_arg_retref, ZEND_ACC_PUBLIC)
+    ZEND_FE_END
 };
 
 
@@ -245,8 +296,10 @@ PHP_MINIT_FUNCTION(node_php_jsobject_class) {
     node_php_jsobject_handlers.clone_obj = NULL;
     node_php_jsobject_handlers.cast_object = NULL;
     node_php_jsobject_handlers.get_property_ptr_ptr = NULL;
+#if !USE_MAGIC_ISSET
     node_php_jsobject_handlers.has_property = node_php_jsobject_has_property;
-    node_php_jsobject_handlers.read_property = node_php_jsobject_read_property;
+#endif
+    //node_php_jsobject_handlers.read_property = node_php_jsobject_read_property;
     /*
     node_php_jsobject_handlers.write_property = node_php_jsobject_write_property;
     node_php_jsobject_handlers.unset_property = node_php_jsobject_unset_property;
diff --git a/src/values.h b/src/values.h
@@ -60,7 +60,14 @@ class ZVal : public NonAssignable {
     // Support a PHP calling convention where the actual zval object
     // is owned by the caller, but the contents are transferred to the
     // callee.
-    inline zval * Transfer() { transferred=true; return zvalp; }
+    inline zval * Transfer(TSRMLS_D) {
+        if (IsObject()) {
+            zend_objects_store_add_ref(zvalp TSRMLS_CC);
+        } else {
+            transferred=true;
+        }
+        return zvalp;
+    }
     inline zval * operator*() const { return Ptr(); } // shortcut
     inline int Type() { return Z_TYPE_P(zvalp); }
     inline bool IsNull() { return Type() == IS_NULL; }
@@ -180,10 +187,15 @@ class ZVal : public NonAssignable {
         // an "owned string", will copy data on creation and free it on delete.
     public:
         explicit OStr(const char *data, std::size_t length)
-            : Str(estrndup(data, length+1), length) { }
+            : Str(NULL, length) {
+            char *ndata = new char[length+1];
+            memcpy(ndata, data, length);
+            ndata[length] = 0;
+            data_ = ndata;
+        }
         virtual ~OStr() {
             if (data_) {
-                efree(const_cast<char*>(data_));
+                delete[] data_;
             }
         }
     };
diff --git a/test/context.js b/test/context.js
@@ -52,4 +52,71 @@ describe('Passing context object from JS to PHP', function() {
 			);
 		});
     });
+	it('should implement isset(), empty(), and property_exists', function() {
+		var out = new StringStream();
+		return php.request({
+			file: path.join(__dirname, 'context2.php'),
+			stream: out,
+			context: {
+				a: 0,
+				b: 42,
+				c: null,
+				d: undefined,
+				e: '0',
+				f: '1'
+			}
+		}).then(function(v) {
+			out.toString().should.equal(
+				'a: int(0)\n' +
+				'isset: bool(true)\n' +
+				'empty: bool(true)\n' +
+				'exists: bool(true)\n' +
+				'\n' +
+				'b: int(42)\n' +
+				'isset: bool(true)\n' +
+				'empty: bool(false)\n' +
+				'exists: bool(true)\n' +
+				'\n' +
+				'c: NULL\n' +
+				'isset: bool(false)\n' +
+				'empty: bool(true)\n' +
+				'exists: bool(true)\n' +
+				'\n' +
+				'd: NULL\n' +
+				'isset: bool(false)\n' +
+				'empty: bool(true)\n' +
+				'exists: bool(true)\n' +
+				'\n' +
+				'e: string(1) "0"\n' +
+				'isset: bool(true)\n' +
+				'empty: bool(true)\n' +
+				'exists: bool(true)\n' +
+				'\n' +
+				'f: string(1) "1"\n' +
+				'isset: bool(true)\n' +
+				'empty: bool(false)\n' +
+				'exists: bool(true)\n' +
+				'\n' +
+				'g: NULL\n' +
+				'isset: bool(false)\n' +
+				'empty: bool(true)\n' +
+				'exists: bool(false)\n' +
+				'\n'
+			);
+		});
+    });
+	it('should handle exceptions in getters', function() {
+		var out = new StringStream();
+		var context = {};
+		Object.defineProperty(context, 'a', { get: function() {
+			throw new Error('boo');
+		} });
+		return php.request({
+			source: "call_user_func(function () { try { var_dump($_SERVER['CONTEXT']->a); } catch (Exception $e) { echo 'exception caught'; } })",
+			stream: out,
+			context: context
+		}).then(function() {
+			out.toString().should.equal('exception caught');
+		});
+	});
 });
diff --git a/test/context2.php b/test/context2.php
@@ -0,0 +1,46 @@
+<?php
+$c = $_SERVER['CONTEXT'];
+
+echo "a: "; var_dump($c->a);
+echo "isset: "; var_dump(isset($c->a));
+echo "empty: "; var_dump(empty($c->a));
+echo "exists: "; var_dump(property_exists($c, "a"));
+echo "\n";
+
+echo "b: "; var_dump($c->b);
+echo "isset: "; var_dump(isset($c->b));
+echo "empty: "; var_dump(empty($c->b));
+echo "exists: "; var_dump(property_exists($c, "b"));
+echo "\n";
+
+echo "c: "; var_dump($c->c);
+echo "isset: "; var_dump(isset($c->c));
+echo "empty: "; var_dump(empty($c->c));
+echo "exists: "; var_dump(property_exists($c, "c"));
+echo "\n";
+
+echo "d: "; var_dump($c->d);
+echo "isset: "; var_dump(isset($c->d));
+echo "empty: "; var_dump(empty($c->d));
+echo "exists: "; var_dump(property_exists($c, "d"));
+echo "\n";
+
+echo "e: "; var_dump($c->e);
+echo "isset: "; var_dump(isset($c->e));
+echo "empty: "; var_dump(empty($c->e));
+echo "exists: "; var_dump(property_exists($c, "e"));
+echo "\n";
+
+echo "f: "; var_dump($c->f);
+echo "isset: "; var_dump(isset($c->f));
+echo "empty: "; var_dump(empty($c->f));
+echo "exists: "; var_dump(property_exists($c, "f"));
+echo "\n";
+
+echo "g: "; var_dump($c->g);
+echo "isset: "; var_dump(isset($c->g));
+echo "empty: "; var_dump(empty($c->g));
+echo "exists: "; var_dump(property_exists($c, "g"));
+echo "\n";
+
+?>
