Prevent potential use-after-free when lowering objects (mozilla#1797)

Changed the FFI for lowering objects avoid the scenario layed out in the
issue.  The new system is that the foreign code calls `inc_ref` on the
handle, then the Rust code calls `remove` on it.  This effectively
makes it so `lower` returns an owned handle instead of a borrowed one.

This replaces a lot of Kotlin code that was concerned about the same
issue in a similar situation.  Removed the `test_handlerace.kts` test
for this, I believe this is now all handled at a lower level and we
don't need this test.  FWIW, the test was failing, but just because we
now raise a different exception -- and the new exception mentions that you
may be using the handle after it's free.

CHANGELOG.md

@@ -14,6 +14,9 @@
 
 [All changes in [[UnreleasedUniFFIVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.25.0...HEAD).
 
+### What's Fixed
+- Fixed potential use-after-free error when lowering object handles (#1797)
+
 ### What's changed?
 
 - The object handle FFI has changed.  External bindings generators will need to update their code to

fixtures/coverall/tests/test_generated_bindings.rs

@@ -3,5 +3,4 @@ uniffi::build_foreign_language_testcases!(
     "tests/bindings/test_coverall.kts",
     "tests/bindings/test_coverall.rb",
     "tests/bindings/test_coverall.swift",
-    "tests/bindings/test_handlerace.kts",
 );

uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt

@@ -34,130 +34,21 @@ inline fun <T : Disposable?, R> T.use(block: (T) -> R) =
 //
 // This class provides core operations for working with the Rust handle to the live Rust struct on
 // the other side of the FFI.
-//
-// There's some subtlety here, because we have to be careful not to operate on a Rust
-// struct after it has been dropped, and because we must expose a public API for freeing
-// the Kotlin wrapper object in lieu of reliable finalizers. The core requirements are:
-//
-//   * Each `FFIObject` instance holds an opaque handle to the underlying Rust struct.
-//     Method calls need to read this handle from the object's state and pass it in to
-//     the Rust FFI.
-//
-//   * When an `FFIObject` is no longer needed, its handle should be passed to a
-//     special destructor function provided by the Rust FFI, which will drop the
-//     underlying Rust struct.
-//
-//   * Given an `FFIObject` instance, calling code is expected to call the special
-//     `destroy` method in order to free it after use, either by calling it explicitly
-//     or by using a higher-level helper like the `use` method. Failing to do so will
-//     leak the underlying Rust struct.
-//
-//   * We can't assume that calling code will do the right thing, and must be prepared
-//     to handle Kotlin method calls executing concurrently with or even after a call to
-//     `destroy`, and to handle multiple (possibly concurrent!) calls to `destroy`.
-//
-//   * We must never allow Rust code to operate on the underlying Rust struct after
-//     the destructor has been called, and must never call the destructor more than once.
-//     Doing so may trigger memory unsafety.
-//
-// If we try to implement this with mutual exclusion on access to the handle, there is the
-// possibility of a race between a method call and a concurrent call to `destroy`:
-//
-//    * Thread A starts a method call, reads the value of the handle, but is interrupted
-//      before it can pass the handle over the FFI to Rust.
-//    * Thread B calls `destroy` and frees the underlying Rust struct.
-//    * Thread A resumes, passing the already-read handle value to Rust and triggering
-//      a use-after-free.
-//
-// One possible solution would be to use a `ReadWriteLock`, with each method call taking
-// a read lock (and thus allowed to run concurrently) and the special `destroy` method
-// taking a write lock (and thus blocking on live method calls). However, we aim not to
-// generate methods with any hidden blocking semantics, and a `destroy` method that might
-// block if called incorrectly seems to meet that bar.
-//
-// So, we achieve our goals by giving each `FFIObject` an associated `AtomicLong` counter to track
-// the number of in-flight method calls, and an `AtomicBoolean` flag to indicate whether `destroy`
-// has been called. These are updated according to the following rules:
-//
-//    * The initial value of the counter is 1, indicating a live object with no in-flight calls.
-//      The initial value for the flag is false.
-//
-//    * At the start of each method call, we atomically check the counter.
-//      If it is 0 then the underlying Rust struct has already been destroyed and the call is aborted.
-//      If it is nonzero them we atomically increment it by 1 and proceed with the method call.
-//
-//    * At the end of each method call, we atomically decrement and check the counter.
-//      If it has reached zero then we destroy the underlying Rust struct.
-//
-//    * When `destroy` is called, we atomically flip the flag from false to true.
-//      If the flag was already true we silently fail.
-//      Otherwise we atomically decrement and check the counter.
-//      If it has reached zero then we destroy the underlying Rust struct.
-//
-// Astute readers may observe that this all sounds very similar to the way that Rust's `Arc<T>` works,
-// and indeed it is, with the addition of a flag to guard against multiple calls to `destroy`.
-//
-// The overall effect is that the underlying Rust struct is destroyed only when `destroy` has been
-// called *and* all in-flight method calls have completed, avoiding violating any of the expectations
-// of the underlying Rust code.
-//
-// In the future we may be able to replace some of this with automatic finalization logic, such as using
-// the new "Cleaner" functionaility in Java 9. The above scheme has been designed to work even if `destroy` is
-// invoked by garbage-collection machinery rather than by calling code (which by the way, it's apparently also
-// possible for the JVM to finalize an object while there is an in-flight call to one of its methods [1],
-// so there would still be some complexity here).
-//
-// Sigh...all of this for want of a robust finalization mechanism.
-//
-// [1] https://stackoverflow.com/questions/24376768/can-java-finalize-an-object-when-it-is-still-in-scope/24380219
-//
-abstract class FFIObject(
-    internal val handle: UniffiHandle
-): Disposable, AutoCloseable {
-
+abstract class FFIObject(): Disposable, AutoCloseable {
     private val wasDestroyed = AtomicBoolean(false)
-    private val callCounter = AtomicLong(1)
 
     open protected fun freeRustArcPtr() {
         // To be overridden in subclasses.
     }
 
     override fun destroy() {
-        // Only allow a single call to this method.
-        // TODO: maybe we should log a warning if called more than once?
         if (this.wasDestroyed.compareAndSet(false, true)) {
-            // This decrement always matches the initial count of 1 given at creation time.
-            if (this.callCounter.decrementAndGet() == 0L) {
-                this.freeRustArcPtr()
-            }
+            this.freeRustArcPtr()
         }
     }
 
     @Synchronized
     override fun close() {
         this.destroy()
     }
-
-    internal inline fun <R> callWithHandle(block: (handle: UniffiHandle) -> R): R {
-        // Check and increment the call counter, to keep the object alive.
-        // This needs a compare-and-set retry loop in case of concurrent updates.
-        do {
-            val c = this.callCounter.get()
-            if (c == 0L) {
-                throw IllegalStateException("${this.javaClass.simpleName} object has already been destroyed")
-            }
-            if (c == Long.MAX_VALUE) {
-                throw IllegalStateException("${this.javaClass.simpleName} call counter would overflow")
-            }
-        } while (! this.callCounter.compareAndSet(c, c + 1L))
-        // Now we can safely do the method call without the handle being freed concurrently.
-        try {
-            return block(this.handle)
-        } finally {
-            // This decrement always matches the increment we performed above.
-            if (this.callCounter.decrementAndGet() == 0L) {
-                this.freeRustArcPtr()
-            }
-        }
-    }
 }

uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt

@@ -6,8 +6,8 @@
 {% include "Interface.kt" %}
 
 class {{ impl_class_name }}(
-    handle: UniffiHandle
-) : FFIObject(handle), {{ interface_name }}{
+    internal val handle: UniffiHandle
+) : FFIObject(), {{ interface_name }}{
 
     {%- match obj.primary_constructor() %}
     {%- when Some with (cons) %}
@@ -30,6 +30,13 @@ class {{ impl_class_name }}(
         }
     }
 
+    internal fun uniffiCloneHandle(): UniffiHandle {
+        rustCall() { status ->
+            _UniFFILib.INSTANCE.{{ obj.ffi_object_inc_ref().name() }}(handle, status)
+        }
+        return handle
+    }
+
     {% for meth in obj.methods() -%}
     {%- match meth.throws_type() -%}
     {%- when Some with (throwable) %}
@@ -40,12 +47,10 @@ class {{ impl_class_name }}(
     @Suppress("ASSIGNED_BUT_NEVER_ACCESSED_VARIABLE")
     override suspend fun {{ meth.name()|fn_name }}({%- call kt::arg_list_decl(meth) -%}){% match meth.return_type() %}{% when Some with (return_type) %} : {{ return_type|type_name }}{% when None %}{%- endmatch %} {
         return uniffiRustCallAsync(
-            callWithHandle { uniffiHandle ->
-                _UniFFILib.INSTANCE.{{ meth.ffi_func().name() }}(
-                    uniffiHandle,
-                    {% call kt::arg_list_lowered(meth) %}
-                )
-            },
+            _UniFFILib.INSTANCE.{{ meth.ffi_func().name() }}(
+                uniffiCloneHandle(),
+                {% call kt::arg_list_lowered(meth) %}
+            ),
             {{ meth|async_poll(ci) }},
             {{ meth|async_complete(ci) }},
             {{ meth|async_free(ci) }},
@@ -69,17 +74,13 @@ class {{ impl_class_name }}(
     {%- match meth.return_type() -%}
     {%- when Some with (return_type) -%}
     override fun {{ meth.name()|fn_name }}({% call kt::arg_list_protocol(meth) %}): {{ return_type|type_name }} =
-        callWithHandle {
-            {%- call kt::to_ffi_call_with_prefix("it", meth) %}
-        }.let {
+        {%- call kt::to_ffi_call_with_prefix("uniffiCloneHandle()", meth) %}.let {
             {{ return_type|lift_fn }}(it)
         }
 
     {%- when None -%}
     override fun {{ meth.name()|fn_name }}({% call kt::arg_list_protocol(meth) %}) =
-        callWithHandle {
-            {%- call kt::to_ffi_call_with_prefix("it", meth) %}
-        }
+        {%- call kt::to_ffi_call_with_prefix("uniffiCloneHandle()", meth) %}
     {% endmatch %}
     {% endif %}
     {% endfor %}
@@ -111,7 +112,7 @@ public object {{ obj|ffi_converter_name }}: FfiConverter<{{ type_name }}, Uniffi
     override fun lower(value: {{ type_name }}): UniffiHandle {
         {%- match obj.imp() %}
         {%- when ObjectImpl::Struct %}
-        return value.handle
+        return value.uniffiCloneHandle()
         {%- when ObjectImpl::Trait %}
         return UniffiHandle(handleMap.insert(value))
         {%- endmatch %}

uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py

@@ -21,6 +21,10 @@ def __del__(self):
         if handle is not None:
             _rust_call(_UniffiLib.{{ obj.ffi_object_free().name() }}, handle)
 
+    def _uniffi_clone_handle(self):
+        _rust_call(_UniffiLib.{{ obj.ffi_object_inc_ref().name() }}, self._uniffi_handle)
+        return self._uniffi_handle
+
     # Used by alternative constructors or any methods which return this type.
     @classmethod
     def _make_instance_(cls, handle):
@@ -89,7 +93,7 @@ def lower(value: {{ protocol_name }}):
         {%- when ObjectImpl::Struct %}
         if not isinstance(value, {{ impl_name }}):
             raise TypeError("Expected {{ impl_name }} instance, {} found".format(type(value).__name__))
-        return value._uniffi_handle
+        return value._uniffi_clone_handle()
         {%- when ObjectImpl::Trait %}
         return {{ ffi_converter_name }}._handle_map.insert(value)
         {%- endmatch %}

uniffi_bindgen/src/bindings/python/templates/macros.py

@@ -104,7 +104,7 @@ def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}):
         {%- call setup_args_extra_indent(meth) %}
         return _uniffi_rust_call_async(
             _UniffiLib.{{ meth.ffi_func().name() }}(
-                self._uniffi_handle, {% call arg_list_lowered(meth) %}
+                self._uniffi_clone_handle(), {% call arg_list_lowered(meth) %}
             ),
             _UniffiLib.{{ meth.ffi_rust_future_poll(ci) }},
             _UniffiLib.{{ meth.ffi_rust_future_complete(ci) }},
@@ -133,14 +133,14 @@ def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}):
     def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}) -> "{{ return_type|type_name }}":
         {%- call setup_args_extra_indent(meth) %}
         return {{ return_type|lift_fn }}(
-            {% call to_ffi_call_with_prefix("self._uniffi_handle", meth) %}
+            {% call to_ffi_call_with_prefix("self._uniffi_clone_handle()", meth) %}
         )
 
 {%-         when None %}
 
     def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}):
         {%- call setup_args_extra_indent(meth) %}
-        {% call to_ffi_call_with_prefix("self._uniffi_handle", meth) %}
+        {% call to_ffi_call_with_prefix("self._uniffi_clone_handle()", meth) %}
 {%      endmatch %}
 {%  endif %}
 

uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb

@@ -21,14 +21,22 @@ def self._uniffi_define_finalizer_by_handle(handle, object_id)
     end
   end
 
+  def _uniffi_clone_handle()
+    {{ ci.namespace()|class_name_rb }}.rust_call(
+      :{{ obj.ffi_object_inc_ref().name() }},
+      @handle
+    )
+    return @handle
+  end
+
   # A private helper for lowering instances into a handle.
   # This does an explicit typecheck, because accidentally lowering a different type of
   # object in a place where this type is expected, could lead to memory unsafety.
   def self._uniffi_lower(inst)
     if not inst.is_a? self
       raise TypeError.new "Expected a {{ obj.name()|class_name_rb }} instance, got #{inst}"
     end
-    return inst.instance_variable_get :@handle
+    return inst._uniffi_clone_handle()
   end
 
   {%- match obj.primary_constructor() %}
@@ -58,14 +66,14 @@ def self.{{ cons.name()|fn_name_rb }}({% call rb::arg_list_decl(cons) %})
   {%- when Some with (return_type) -%}
   def {{ meth.name()|fn_name_rb }}({% call rb::arg_list_decl(meth) %})
     {%- call rb::coerce_args_extra_indent(meth) %}
-    result = {% call rb::to_ffi_call_with_prefix("@handle", meth) %}
+    result = {% call rb::to_ffi_call_with_prefix("_uniffi_clone_handle()", meth) %}
     return {{ "result"|lift_rb(return_type) }}
   end
 
   {%- when None -%}
   def {{ meth.name()|fn_name_rb }}({% call rb::arg_list_decl(meth) %})
       {%- call rb::coerce_args_extra_indent(meth) %}
-      {% call rb::to_ffi_call_with_prefix("@handle", meth) %}
+      {% call rb::to_ffi_call_with_prefix("_uniffi_clone_handle()", meth) %}
   end
   {% endmatch %}
   {% endfor %}