Add thread checks to emscripten::val (#20344)

In multithreading environment using `emscripten::val` is very error-prone and hard to debug. The reason for that is that, on one hand, `val` represents JavaScript values which cannot be (easily) shared between threads, but, on another, `val` itself is a thin wrapper around an integer handle which can be shared across threads way too easily.

As a result, accessing `val` from the wrong thread sometimes succeeds by accident (such as for built-in constants like `undefined`/`false`/`true`), sometimes crashes with invalid handle, and sometimes just quietly returns completely different JS value because a handle with the same ID happened to be valid on both threads, but points to different underlying values.

In the last scenario code can even keep going forward for some time, using and making changes on the incorrect JS value before it's apparent that something went very wrong.

To make it easier to catch such cases, in this PR I'm adding a `pthread_t` field to the `val` class that stores the owner thread of the given handle. User can still store `val` and send it across threads, but the moment they try to operate on the underlying JS value on the wrong thread, an assertion will fire with a clear message.

This check is activated only when code is compiled with pthread support, and only if user didn't disable assertions via `NDEBUG` (we can't use the `-sASSERTIONS` setting here because it's a header not compiled code). This still adds a tiny size overhead if the user is not passing `-DNDEBUG`, but typical build systems (e.g. CMake) pass it automatically in release builds, and overall I believe benefits outweigh the overhead in this scenario.

In the future we might also automatically proxy all operations to the correct thread so that `emscripten::val` behaves like any other native object. However, that's out of scope of this particular PR, so for now just laying the groundwork and making those failure scenarios more visible.

Author: RReverser

ChangeLog.md

@@ -31,6 +31,9 @@ See docs/process.md for more on how version tagging works.
   mirrors the existing `EM_ASM_PTR`. (#20261)
 - Emscripten now implements default POSIX signal handlers. These can
   terminate or abort the program in response to `raise` cals. (#20257)
+- `emscripten::val` now prevents accidental access to the underlying JavaScript
+  value from threads other than its owner. This already didn't work correctly
+  in majority of cases, but now it will throw a clear assertion failure. (#20344)
 
 3.1.46 - 09/15/23
 -----------------

system/include/emscripten/val.h

@@ -386,116 +386,119 @@ class val {
   explicit val(T&& value) {
     using namespace internal;
 
-    typedef internal::BindingType<T> BT;
     WireTypePack<T> argv(std::forward<T>(value));
-    handle = _emval_take_value(internal::TypeID<T>::get(), argv);
+    new (this) val(_emval_take_value(internal::TypeID<T>::get(), argv));
   }
 
-  val() : handle(EM_VAL(internal::_EMVAL_UNDEFINED)) {}
+  val() : val(EM_VAL(internal::_EMVAL_UNDEFINED)) {}
 
   explicit val(const char* v)
-      : handle(internal::_emval_new_cstring(v))
+      : val(internal::_emval_new_cstring(v))
   {}
 
-  val(val&& v) : handle(v.handle) {
+  // Note: unlike other constructors, this doesn't use as_handle() because
+  // it just moves a value and doesn't need to go via incref/decref.
+  // This means it's safe to move values across threads - an error will
+  // only arise if you access or free it from the wrong thread later.
+  val(val&& v) : handle(v.handle), thread(v.thread) {
     v.handle = 0;
   }
 
-  val(const val& v) : handle(v.handle) {
+  val(const val& v) : val(v.as_handle()) {
     internal::_emval_incref(handle);
   }
 
   ~val() {
-    internal::_emval_decref(handle);
+    if (EM_VAL handle = as_handle()) {
+      internal::_emval_decref(handle);
+      handle = 0;
+    }
   }
 
   EM_VAL as_handle() const {
+#ifdef _REENTRANT
+    assert(pthread_equal(thread, pthread_self()) && "val accessed from wrong thread");
+#endif
     return handle;
   }
 
   val& operator=(val&& v) & {
-    auto v_handle = v.handle;
-    v.handle = 0;
-    if (handle) {
-      internal::_emval_decref(handle);
-    }
-    handle = v_handle;
+    val tmp(std::move(v));
+    this->~val();
+    new (this) val(std::move(tmp));
     return *this;
   }
 
   val& operator=(const val& v) & {
-    internal::_emval_incref(v.handle);
-    internal::_emval_decref(handle);
-    handle = v.handle;
-    return *this;
+    return *this = val(v);
   }
 
   bool hasOwnProperty(const char* key) const {
     return val::global("Object")["prototype"]["hasOwnProperty"].call<bool>("call", *this, val(key));
   }
 
   bool isNull() const {
-    return handle == EM_VAL(internal::_EMVAL_NULL);
+    return as_handle() == EM_VAL(internal::_EMVAL_NULL);
   }
 
   bool isUndefined() const {
-    return handle == EM_VAL(internal::_EMVAL_UNDEFINED);
+    return as_handle() == EM_VAL(internal::_EMVAL_UNDEFINED);
   }
 
   bool isTrue() const {
-    return handle == EM_VAL(internal::_EMVAL_TRUE);
+    return as_handle() == EM_VAL(internal::_EMVAL_TRUE);
   }
 
   bool isFalse() const {
-    return handle == EM_VAL(internal::_EMVAL_FALSE);
+    return as_handle() == EM_VAL(internal::_EMVAL_FALSE);
   }
 
   bool isNumber() const {
-    return internal::_emval_is_number(handle);
+    return internal::_emval_is_number(as_handle());
   }
 
   bool isString() const {
-    return internal::_emval_is_string(handle);
+    return internal::_emval_is_string(as_handle());
   }
 
   bool isArray() const {
     return instanceof(global("Array"));
   }
 
   bool equals(const val& v) const {
-    return internal::_emval_equals(handle, v.handle);
+    return internal::_emval_equals(as_handle(), v.as_handle());
   }
 
   bool operator==(const val& v) const {
-    return internal::_emval_equals(handle, v.handle);
+    return internal::_emval_equals(as_handle(), v.as_handle());
   }
 
   bool operator!=(const val& v) const {
     return !(*this == v);
   }
 
   bool strictlyEquals(const val& v) const {
-    return internal::_emval_strictly_equals(handle, v.handle);
+    return internal::_emval_strictly_equals(as_handle(), v.as_handle());
   }
 
   bool operator>(const val& v) const {
-    return internal::_emval_greater_than(handle, v.handle);
+    return internal::_emval_greater_than(as_handle(), v.as_handle());
   }
 
   bool operator>=(const val& v) const {
     return (*this > v) || (*this == v);
   }
 
   bool operator<(const val& v) const {
-    return internal::_emval_less_than(handle, v.handle);
+    return internal::_emval_less_than(as_handle(), v.as_handle());
   }
 
   bool operator<=(const val& v) const {
     return (*this < v) || (*this == v);
   }
 
   bool operator!() const {
-    return internal::_emval_not(handle);
+    return internal::_emval_not(as_handle());
   }
 
   template<typename... Args>
@@ -505,17 +508,17 @@ class val {
 
   template<typename T>
   val operator[](const T& key) const {
-    return val(internal::_emval_get_property(handle, val_ref(key).handle));
+    return val(internal::_emval_get_property(as_handle(), val_ref(key).as_handle()));
   }
 
   template<typename K, typename V>
   void set(const K& key, const V& value) {
-    internal::_emval_set_property(handle, val_ref(key).handle, val_ref(value).handle);
+    internal::_emval_set_property(as_handle(), val_ref(key).as_handle(), val_ref(value).as_handle());
   }
 
   template<typename T>
   bool delete_(const T& property) const {
-    return internal::_emval_delete(handle, val_ref(property).handle);
+    return internal::_emval_delete(as_handle(), val_ref(property).as_handle());
   }
 
   template<typename... Args>
@@ -527,7 +530,7 @@ class val {
   ReturnValue call(const char* name, Args&&... args) const {
     using namespace internal;
 
-    return MethodCaller<ReturnValue, Args...>::call(handle, name, std::forward<Args>(args)...);
+    return MethodCaller<ReturnValue, Args...>::call(as_handle(), name, std::forward<Args>(args)...);
   }
 
   template<typename T, typename ...Policies>
@@ -539,7 +542,7 @@ class val {
 
     EM_DESTRUCTORS destructors;
     EM_GENERIC_WIRE_TYPE result = _emval_as(
-        handle,
+        as_handle(),
         targetType.getTypes()[0],
         &destructors);
     DestructorsRunner dr(destructors);
@@ -553,7 +556,7 @@ class val {
     typedef BindingType<int64_t> BT;
     typename WithPolicies<>::template ArgTypeList<int64_t> targetType;
 
-    return _emval_as_int64(handle, targetType.getTypes()[0]);
+    return _emval_as_int64(as_handle(), targetType.getTypes()[0]);
   }
 
   template<>
@@ -563,41 +566,41 @@ class val {
     typedef BindingType<uint64_t> BT;
     typename WithPolicies<>::template ArgTypeList<uint64_t> targetType;
 
-    return  _emval_as_uint64(handle, targetType.getTypes()[0]);
+    return  _emval_as_uint64(as_handle(), targetType.getTypes()[0]);
   }
 
 // If code is not being compiled with GNU extensions enabled, typeof() is not a reserved keyword, so support that as a member function.
 #if __STRICT_ANSI__
   val typeof() const {
-    return val(internal::_emval_typeof(handle));
+    return val(internal::_emval_typeof(as_handle()));
   }
 #endif
 
 // Prefer calling val::typeOf() over val::typeof(), since this form works in both C++11 and GNU++11 build modes. "typeof" is a reserved word in GNU++11 extensions.
   val typeOf() const {
-    return val(internal::_emval_typeof(handle));
+    return val(internal::_emval_typeof(as_handle()));
   }
 
   bool instanceof(const val& v) const {
-    return internal::_emval_instanceof(handle, v.handle);
+    return internal::_emval_instanceof(as_handle(), v.as_handle());
   }
 
   bool in(const val& v) const {
-    return internal::_emval_in(handle, v.handle);
+    return internal::_emval_in(as_handle(), v.as_handle());
   }
 
   [[noreturn]] void throw_() const {
-    internal::_emval_throw(handle);
+    internal::_emval_throw(as_handle());
   }
 
   val await() const {
-    return val(internal::_emval_await(handle));
+    return val(internal::_emval_await(as_handle()));
   }
 
 private:
-  // takes ownership, assumes handle already incref'd
+  // takes ownership, assumes handle already incref'd and lives on the same thread
   explicit val(EM_VAL handle)
-      : handle(handle)
+      : handle(handle), thread(pthread_self())
   {}
 
   template<typename WrapperType>
@@ -609,7 +612,7 @@ class val {
 
     WithPolicies<>::ArgTypeList<Args...> argList;
     WireTypePack<Args...> argv(std::forward<Args>(args)...);
-    return val(impl(handle, argList.getCount(), argList.getTypes(), argv));
+    return val(impl(as_handle(), argList.getCount(), argList.getTypes(), argv));
   }
 
   template<typename T>
@@ -621,6 +624,7 @@ class val {
     return v;
   }
 
+  pthread_t thread;
   EM_VAL handle;
 
   friend struct internal::BindingType<val>;
@@ -640,8 +644,9 @@ struct BindingType<T, typename std::enable_if<std::is_base_of<val, T>::value &&
                                               !std::is_const<T>::value>::type> {
   typedef EM_VAL WireType;
   static WireType toWireType(const val& v) {
-    _emval_incref(v.handle);
-    return v.handle;
+    EM_VAL handle = v.as_handle();
+    _emval_incref(handle);
+    return handle;
   }
   static val fromWireType(WireType v) {
     return val::take_ownership(v);
@@ -652,11 +657,11 @@ struct BindingType<T, typename std::enable_if<std::is_base_of<val, T>::value &&
 
 template <typename T, typename... Policies>
 std::vector<T> vecFromJSArray(const val& v, Policies... policies) {
-  const size_t l = v["length"].as<size_t>();
+  const uint32_t l = v["length"].as<uint32_t>();
 
   std::vector<T> rv;
   rv.reserve(l);
-  for (size_t i = 0; i < l; ++i) {
+  for (uint32_t i = 0; i < l; ++i) {
     rv.push_back(v[i].as<T>(std::forward<Policies>(policies)...));
   }
 

test/test_core.py

@@ -7776,6 +7776,34 @@ def test_embind_val_assignment(self):
     err = self.expect_fail([EMCC, test_file('embind/test_val_assignment.cpp'), '-lembind', '-c'])
     self.assertContained('candidate function not viable: expects an lvalue for object argument', err)
 
+  @node_pthreads
+  def test_embind_val_cross_thread(self):
+    self.emcc_args += ['--bind']
+    self.setup_node_pthreads()
+    create_file('test_embind_val_cross_thread.cpp', r'''
+      #include <emscripten.h>
+      #include <emscripten/val.h>
+      #include <thread>
+      #include <stdio.h>
+
+      using emscripten::val;
+
+      int main(int argc, char **argv) {
+        // Store a value handle from the main thread.
+        val value(0);
+        std::thread([&] {
+          // Set to a value handle from a different thread.
+          value = val(1);
+        }).join();
+        // Try to access the stored handle from the main thread.
+        // Without the check (if compiled with -DNDEBUG) this will incorrectly
+        // print "0" instead of "1" since the handle with the same ID
+        // resolves to different values on different threads.
+        printf("%d\n", value.as<int>());
+      }
+    ''')
+    self.do_runf('test_embind_val_cross_thread.cpp', 'val accessed from wrong thread', assert_returncode=NON_ZERO)
+
   def test_embind_dynamic_initialization(self):
     self.emcc_args += ['-lembind']
     self.do_run_in_out_file_test('embind/test_dynamic_initialization.cpp')