Enforce the ArrayBuffer borrow contract for Java TurboModules (#57982)

Summary:
Pull Request resolved: #57982

Changelog: [ANDROID][FIXED]  Enforce the ArrayBuffer borrow contract for Java TurboModules

Follow-up hardening for the Android `ArrayBuffer` TurboModule type. Three problems:

**1. Borrowed JS-heap bytes outlived the call that lent them.** For a synchronous
method, `convertJSIArgsToJNIArgs` hands the module a `ByteBuffer` aliasing the JS
`ArrayBuffer`'s bytes without copying. Nothing stopped a module from stashing that
`ArrayBuffer` in a field and reading it later, after the JS heap may have moved,
freed, or reused the memory — a use-after-free that reads as intermittent data
corruption rather than a crash.

The borrow is now explicitly scoped to the call frame. `JNIArgs` records every
borrowed `ArrayBuffer` and revokes it in its destructor — including when the call
throws — via the new `JArrayBuffer::invalidate`, which drops the C++ side's
reference to the bytes. `ArrayBuffer.bytes` and `ArrayBuffer.size` then throw,
with a message pointing at `ArrayBuffer.arrayBufferWithCopiedBytes`, and
`JArrayBuffer::toJSBuffer` throws rather than aliasing revoked memory. Modules
that need the bytes past the call copy them; modules that don't keep the
zero-copy fast path.

Revocation lives entirely on the C++ side: the peer is the single source of
truth, and Kotlin asks it through `isBytesValid`. The destructor runs while the
stack unwinds, possibly with a Java exception pending, so it resolves each peer
pointer at borrow time — the `global_ref` alongside it keeps the Java object, and
therefore the peer, alive — and calls only the `noexcept`
`JArrayBuffer::invalidate`. No JNI calls are made from the destructor, which is
what lets it stay `noexcept` honestly.

**2. Argument conversion aborted under runtimes that refuse `tryGetMutableBuffer`.**
`jsi::Runtime::tryGetMutableBuffer` and `detached` are not universally
implemented: tracing and replay runtimes throw from `tryGetMutableBuffer`, and
`detached` throws a `JSINativeException` if the JS-side property isn't a bool.
`ArrayBuffer` argument conversion is not wrapped in a try/catch, so either throw
propagated out of a JNI frame. Both calls now go through exception-tolerant
helpers in `react/bridging/ArrayBuffer.h`; a runtime that refuses to answer is
treated as "no native buffer available", which selects the copy path. Routing
`AsyncArrayBuffer::acquire` and `::borrow` through the same helper fixes the
identical latent bug on the shared C++/ObjC path.

**3. A wrong return type from a module crashed instead of raising a JS error.**
The `ArrayBufferKind` return path cast the returned `jobject` to `JArrayBuffer`
unconditionally. A module returning any other object type produced undefined
behavior. The cast is now guarded by an `isInstanceOf` check that throws a
`jsi::JSError` naming the offending module and method.

Also in this change:
- `JByteBufferMutableBuffer::data()` reports null for a zero-capacity direct
  buffer instead of calling `getDirectBytes()`, which throws for one. That made
  `createArrayBuffer` throw for an empty `ArrayBuffer`.
- Dropped two dead zero-size branches in `JArrayBuffer`: `JByteBuffer::wrapBytes`
  already routes `size == 0` to an empty buffer.
- `JArrayBuffer.cpp` reuses the shared `detail::OwnedBytesBuffer` from
  `react/bridging/ArrayBuffer.h` instead of a second local copy.
- `ArrayBuffer.kt` KDoc corrected: the returned JS `ArrayBuffer` is a new object
  over the same bytes rather than the identical one, `size` is the capacity and
  not a view's remaining bytes, and `arrayBufferWithOwnedBytes` documents the
  caller's lifetime obligation.
- `ArrayBuffer.kt` moves from the `bridge` target to `native-types`, alongside the
  other JNI-backed bridge types.

Changelog:
[Android][Breaking] - TurboModule methods taking or returning an `ArrayBuffer`
now use `com.facebook.react.bridge.ArrayBuffer` instead of
`java.nio.ByteBuffer`, and an `ArrayBuffer` argument must not be retained past
the method that receives it unless its bytes are copied with
`ArrayBuffer.arrayBufferWithCopiedBytes()`.

Reviewed By: javache

Differential Revision: D115794808

fbshipit-source-id: 26f5d863469cc14a3f1bffc2cbc3302f3e983ecb

Author: christophpurrer

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ArrayBuffer.kt

@@ -14,11 +14,16 @@ import java.nio.ByteBuffer
 /**
  * A fixed-length byte buffer for TurboModule `ArrayBuffer` arguments and return values.
  *
+ * Returning an owning [ArrayBuffer] to JS produces a *new* JS `ArrayBuffer` object over the same
+ * bytes, so a module that mutates an argument in place and returns it gives JS a different object
+ * that aliases the memory it passed in. Returning a non-owning [ArrayBuffer] gives JS a copy.
+ *
  * @property isOwningBytes:
  * - `true` — safe to retain and return to JS. Synchronize externally if JS may touch the same
  *   memory concurrently.
- * - `false` — bytes are borrowed from a JS `ArrayBuffer` for the current synchronous call only.
- *   Copy with [arrayBufferWithCopiedBytes] to keep them.
+ * - `false` — the bytes are borrowed from a JS `ArrayBuffer` and are only valid until the method
+ *   that received this [ArrayBuffer] returns. After that, [bytes] and [size] throw. Copy with
+ *   [arrayBufferWithCopiedBytes] to keep them.
  */
 @DoNotStrip
 public class ArrayBuffer : HybridClassBase {
@@ -39,14 +44,50 @@ public class ArrayBuffer : HybridClassBase {
     initHybrid(buffer, isOwningBytes)
   }
 
+  /**
+   * The bytes as a direct [ByteBuffer]. The buffer is shared, not a copy: its position and limit
+   * belong to the caller, so duplicate it rather than relying on where a previous reader left it.
+   *
+   * Reading this property is the only access path that is checked against revocation, and the
+   * returned [ByteBuffer] is not. Its address cannot be cleared once handed out, so a [ByteBuffer]
+   * kept past the method that received a non-owning [ArrayBuffer] reads freed or relocated memory
+   * with no exception to warn you. Do not store it, and do not let it outlive the [ArrayBuffer] it
+   * came from: retain the [ArrayBuffer] and read [bytes] again, or copy the bytes with
+   * [arrayBufferWithCopiedBytes].
+   *
+   * @throws IllegalStateException if the bytes were borrowed and the method that received this
+   *   [ArrayBuffer] has already returned. See [isOwningBytes].
+   */
   public val bytes: ByteBuffer
-    get() = buffer
+    get() {
+      checkBytesValid()
+      return buffer
+    }
 
+  /**
+   * The capacity of the buffer in bytes, independent of the position and limit of [bytes].
+   *
+   * @throws IllegalStateException if the bytes were borrowed and the method that received this
+   *   [ArrayBuffer] has already returned. See [isOwningBytes].
+   */
   public val size: Int
-    get() = buffer.capacity()
+    get() {
+      checkBytesValid()
+      return buffer.capacity()
+    }
+
+  private fun checkBytesValid() {
+    check(isBytesValid()) {
+      "ArrayBuffer: the bytes of a non-owning ArrayBuffer were accessed after the method " +
+          "that received it returned. Copy them with " +
+          "ArrayBuffer.arrayBufferWithCopiedBytes() to use them later."
+    }
+  }
 
   private external fun initHybrid(buffer: ByteBuffer, isOwningBytes: Boolean)
 
+  private external fun isBytesValid(): Boolean
+
   public companion object {
     init {
       ReactNativeJniCommonSoLoader.staticInit()
@@ -69,7 +110,11 @@ public class ArrayBuffer : HybridClassBase {
       return buffer
     }
 
-    /** @param source remaining bytes are copied into a new owning buffer */
+    /**
+     * @param source the bytes between its position and limit are copied into a new owning buffer.
+     *   Pass a buffer positioned at 0 with the limit at its capacity to copy all of it. The
+     *   position and limit of `source` are left unchanged.
+     */
     @JvmStatic
     @DoNotStrip
     public fun arrayBufferWithCopiedBytes(source: ByteBuffer): ArrayBuffer {
@@ -83,8 +128,9 @@ public class ArrayBuffer : HybridClassBase {
     }
 
     /**
-     * @param source copied into a new owning buffer. Use to keep bytes from a non-owning argument
-     *   after the call returns.
+     * @param source all [size] bytes are copied into a new owning buffer, regardless of the
+     *   position and limit of its [bytes]. Use to keep bytes from a non-owning argument after the
+     *   call returns.
      */
     @JvmStatic
     @DoNotStrip
@@ -102,8 +148,12 @@ public class ArrayBuffer : HybridClassBase {
     }
 
     /**
-     * @param buffer direct [ByteBuffer] to alias without copying. The caller must keep it valid for
-     *   as long as this [ArrayBuffer] lives.
+     * Aliases an existing direct [ByteBuffer] without copying. The resulting [ArrayBuffer] reports
+     * [isOwningBytes] as `true`, so it may be retained and returned to JS — but it does not own the
+     * memory: the caller must keep `buffer` and whatever backs it valid for as long as the
+     * [ArrayBuffer], and any JS `ArrayBuffer` derived from it, is reachable.
+     *
+     * @param buffer direct [ByteBuffer] to alias without copying
      */
     @JvmStatic
     @DoNotStrip

packages/react-native/ReactAndroid/src/main/jni/react/jni/CMakeLists.txt

@@ -39,7 +39,7 @@ add_library(
 target_merge_so(reactnativejni_common)
 target_include_directories(reactnativejni_common PUBLIC ../../)
 
-target_link_libraries(reactnativejni_common fbjni folly_runtime react_cxxreact rrc_view)
+target_link_libraries(reactnativejni_common fbjni folly_runtime react_bridging react_cxxreact rrc_view)
 target_compile_reactnative_options(reactnativejni_common PRIVATE)
 target_compile_options(reactnativejni_common PRIVATE -Wno-unused-lambda-capture)
 

packages/react-native/ReactAndroid/src/main/jni/react/jni/JArrayBuffer.cpp

@@ -9,38 +9,29 @@
 
 #include <cstring>
 #include <span>
+#include <stdexcept>
 #include <utility>
 #include <vector>
 
+#include <react/bridging/ArrayBuffer.h>
+
 #include "JByteBufferMutableBuffer.h"
 
 namespace facebook::react {
 
 namespace {
 
-// Holds a copy of bytes borrowed from a JS ArrayBuffer.
-class OwnedBytesBuffer final : public jsi::MutableBuffer {
- public:
-  explicit OwnedBytesBuffer(std::vector<uint8_t> bytes) noexcept
-      : bytes_(std::move(bytes)) {}
-
-  size_t size() const override {
-    return bytes_.size();
-  }
-
-  uint8_t* data() override {
-    return bytes_.data();
-  }
-
- private:
-  std::vector<uint8_t> bytes_;
-};
+const char* const kRevokedBorrowMessage =
+    "com.facebook.react.bridge.ArrayBuffer: the bytes of a non-owning ArrayBuffer "
+    "were accessed after the method that received it returned. Copy them with "
+    "ArrayBuffer.arrayBufferWithCopiedBytes() to use them later.";
 
 } // namespace
 
 void JArrayBuffer::registerNatives() {
   registerHybrid({
       makeNativeMethod("initHybrid", JArrayBuffer::initHybrid),
+      makeNativeMethod("isBytesValid", JArrayBuffer::isBytesValid),
   });
 }
 
@@ -54,6 +45,23 @@ void JArrayBuffer::initHybrid(
       owningBytes != JNI_FALSE);
 }
 
+jboolean JArrayBuffer::isBytesValid() {
+  return hasBytes() ? JNI_TRUE : JNI_FALSE;
+}
+
+void JArrayBuffer::invalidate() noexcept {
+  if (!owningBytes_) {
+    buffer_.reset();
+  }
+}
+
+const std::shared_ptr<jsi::MutableBuffer>& JArrayBuffer::mutableBuffer() const {
+  if (!hasBytes()) {
+    throw std::runtime_error(kRevokedBorrowMessage);
+  }
+  return buffer_;
+}
+
 jni::local_ref<JArrayBuffer::javaobject> JArrayBuffer::create(
     jni::local_ref<jni::JByteBuffer> byteBuffer,
     std::shared_ptr<jsi::MutableBuffer> buffer,
@@ -66,24 +74,15 @@ jni::local_ref<JArrayBuffer::javaobject> JArrayBuffer::create(
 
 jni::local_ref<JArrayBuffer::javaobject> JArrayBuffer::createOwning(
     std::shared_ptr<jsi::MutableBuffer> buffer) {
-  // NewDirectByteBuffer rejects a null address, which is what an empty
-  // jsi::ArrayBuffer reports, so empty buffers get an allocation of their own.
-  if (buffer->size() == 0) {
-    return create(jni::JByteBuffer::allocateDirect(0), std::move(buffer), true);
-  }
-
   auto byteBuffer = jni::JByteBuffer::wrapBytes(buffer->data(), buffer->size());
   return create(std::move(byteBuffer), std::move(buffer), true);
 }
 
 jni::local_ref<JArrayBuffer::javaobject> JArrayBuffer::createUnowned(
     void* bytes,
     size_t size) {
-  // NewDirectByteBuffer rejects a null address, which is what an empty
-  // jsi::ArrayBuffer reports, so empty buffers get an allocation of their own.
-  auto byteBuffer = size == 0
-      ? jni::JByteBuffer::allocateDirect(0)
-      : jni::JByteBuffer::wrapBytes(static_cast<uint8_t*>(bytes), size);
+  auto byteBuffer =
+      jni::JByteBuffer::wrapBytes(static_cast<uint8_t*>(bytes), size);
   auto buffer = std::make_shared<JByteBufferMutableBuffer>(byteBuffer);
   return create(std::move(byteBuffer), std::move(buffer), false);
 }
@@ -102,16 +101,28 @@ jni::local_ref<JArrayBuffer::javaobject> JArrayBuffer::createOwned(
 }
 
 std::shared_ptr<jsi::MutableBuffer> JArrayBuffer::toJSBuffer(
+    jsi::Runtime& runtime,
     jni::alias_ref<javaobject> arrayBuffer) {
   auto* self = arrayBuffer->cthis();
+  // create() runs the Kotlin constructor before setNativePointer, so a Java
+  // ArrayBuffer without a C++ peer is reachable if either step throws.
+  if (self == nullptr) {
+    throw jsi::JSError(
+        runtime, "com.facebook.react.bridge.ArrayBuffer has no native peer.");
+  }
+  if (!self->hasBytes()) {
+    throw jsi::JSError(runtime, kRevokedBorrowMessage);
+  }
+
+  const auto& buffer = self->mutableBuffer();
   if (self->owningBytes_) {
-    return self->buffer_;
+    return buffer;
   }
 
   // Borrowed bytes still belong to the inbound JS ArrayBuffer; copy them before
   // handing a new buffer back to JS.
-  auto bytes = std::span<uint8_t>(self->buffer_->data(), self->buffer_->size());
-  return std::make_shared<OwnedBytesBuffer>(
+  auto bytes = std::span<uint8_t>(buffer->data(), buffer->size());
+  return std::make_shared<detail::OwnedBytesBuffer>(
       std::vector<uint8_t>(bytes.begin(), bytes.end()));
 }
 

packages/react-native/ReactAndroid/src/main/jni/react/jni/JArrayBuffer.h

@@ -40,8 +40,31 @@ class JArrayBuffer : public jni::HybridClass<JArrayBuffer> {
 
   // Convert a module return value for rt.createArrayBuffer. Owning buffers pass
   // through; borrowed ones are copied because createArrayBuffer needs its own
-  // backing store.
-  static std::shared_ptr<jsi::MutableBuffer> toJSBuffer(jni::alias_ref<javaobject> arrayBuffer);
+  // backing store. Raises a jsi::JSError if the buffer has no native peer or
+  // its borrow has been revoked.
+  static std::shared_ptr<jsi::MutableBuffer> toJSBuffer(jsi::Runtime &runtime, jni::alias_ref<javaobject> arrayBuffer);
+
+  // Revokes access to borrowed bytes. Called when the call frame that lent the
+  // bytes unwinds, so a module that retained a non-owning ArrayBuffer gets an
+  // exception instead of reading memory the JS heap has moved or freed. Owning
+  // buffers are unaffected.
+  void invalidate() noexcept;
+
+  // The bytes this buffer was created over. Throws if a borrow has since been
+  // revoked by invalidate().
+  const std::shared_ptr<jsi::MutableBuffer> &mutableBuffer() const;
+
+  // Whether the bytes are still reachable, i.e. this is an owning buffer or a
+  // borrow that invalidate() has not revoked.
+  bool hasBytes() const noexcept
+  {
+    return buffer_ != nullptr;
+  }
+
+  bool isOwningBytes() const noexcept
+  {
+    return owningBytes_;
+  }
 
   JArrayBuffer(std::shared_ptr<jsi::MutableBuffer> buffer, bool owningBytes) noexcept
       : buffer_(std::move(buffer)), owningBytes_(owningBytes)
@@ -54,6 +77,8 @@ class JArrayBuffer : public jni::HybridClass<JArrayBuffer> {
   static void
   initHybrid(jni::alias_ref<jhybridobject> jobj, jni::alias_ref<jni::JByteBuffer> buffer, jboolean owningBytes);
 
+  jboolean isBytesValid();
+
   static jni::local_ref<javaobject>
   create(jni::local_ref<jni::JByteBuffer> byteBuffer, std::shared_ptr<jsi::MutableBuffer> buffer, bool owningBytes);
 

packages/react-native/ReactAndroid/src/main/jni/react/jni/JByteBufferMutableBuffer.h

@@ -45,6 +45,12 @@ class JByteBufferMutableBuffer final : public jsi::MutableBuffer {
   }
   uint8_t *data() override
   {
+    // GetDirectBufferAddress may report null for a zero-capacity direct buffer,
+    // which getDirectBytes turns into an exception. An empty buffer has no bytes
+    // to address, so report that directly instead.
+    if (byteBuffer_->getDirectSize() == 0) {
+      return nullptr;
+    }
     return byteBuffer_->getDirectBytes();
   }