Repository: denoland/deno_core
Pull request #246 (2023-09-29 18:28:02)

fix(ops): correctly handle multiple array buffers in op_deserialize
This PR fixes `op_deserialize` when multiple array buffers are transferred.

fixes  https://github.com/denoland/deno/issues/20727


CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/denoland/deno_core?pullRequest=246) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/denoland/deno_core?pullRequest=246) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/denoland/deno_core?pullRequest=246) it.</sub>
marcosc90: For some reason `op_serialize` work differently in `deno_core` (at least on tests), so can't make the test work in `deno_core` (it passes)

```js

function testIssue20727() {
  // https://github.com/denoland/deno/issues/20727
  Deno.core = Deno.core ?? [Deno.internal].core;
 
  const ab = new ArrayBuffer(10);
  const transferList = [ab]
  Deno.core.ops.op_serialize(
    { ab },
    { transferredArrayBuffers: transferList },
  );
  // transferList is [0] in deno, and [{}] in deno_core
  const data = {
    array1: new Uint32Array([]),
    array2: new Float32Array([]),
  };

  const transferredArrayBuffers = [
    data.array1.buffer,
    data.array2.buffer,
  ];
  const serializedMultipleTransferredBuffers = Deno.core.ops.op_serialize(
    { id: 2, data },
    { transferredArrayBuffers },
  );

  // should not throw
  const x = Deno.core.ops.op_deserialize(
    serializedMultipleTransferredBuffers,
    { transferredArrayBuffers },
  );
}

testIssue20727();
```

`transferList` after `op_serialize` is `[0]` when running from deno, and `[{}]` when running from `deno_core` test file

The above test fails on deno, and works when using this branch.
marcosc90: https://github.com/denoland/deno_core/blob/52a50c1ffecc491e471114fca65144923cea0b90/core/ops_builtin_v8.rs#L402

`state.shared_array_buffer_store` is empty when called from `deno_core` tests which is why I can't test it. Any guidance is appreciated, if not I can place the test in deno.
bartlomieju: @marcosc90 I will take a look at it tomorrow and try to give you some pointers 👍 
lucacasonato: @marcosc90 `deno_core` tests are probably not passing the `shared_array_buffer_store` in `JsRuntimeOptions` 
bartlomieju: @marcosc90 Luca is right here - you'll need to pass `JsRuntimeOptions.shared_array_buffer_store` and the tests should pass at that point. Could you add the test?

Patch:
diff --git a/core/ops_builtin_v8.rs b/core/ops_builtin_v8.rs
index 68b7e6f..d405908 100644
--- a/core/ops_builtin_v8.rs
+++ b/core/ops_builtin_v8.rs
@@ -493,7 +493,7 @@ pub fn op_deserialize<'a>(
           let array_buffer =
             v8::ArrayBuffer::with_backing_store(scope, &backing_store);
           value_deserializer.transfer_array_buffer(id, array_buffer);
-          transferred_array_buffers.set(scope, id_val, array_buffer.into());
+          transferred_array_buffers.set(scope, i, array_buffer.into());
         } else {
           return Err(type_error(
             "transferred array buffer not present in shared_array_buffer_store",


Test patch:
diff --git a/core/runtime/tests/mod.rs b/core/runtime/tests/mod.rs
index daf4a26..1eb1c84 100644
--- a/core/runtime/tests/mod.rs
+++ b/core/runtime/tests/mod.rs
@@ -1,5 +1,6 @@
 use crate as deno_core;
 use crate::error::AnyError;
+use crate::CrossIsolateStore;
 use crate::JsRuntime;
 use crate::OpState;
 use crate::RuntimeOptions;
@@ -82,6 +83,7 @@ fn setup(mode: Mode) -> (JsRuntime, Arc<AtomicUsize>) {
     get_error_class_fn: Some(&|error| {
       crate::error::get_custom_error_class(error).unwrap()
     }),
+    shared_array_buffer_store: Some(CrossIsolateStore::default()),
     ..Default::default()
   });
 
diff --git a/core/runtime/tests/serialize_deserialize_test.js b/core/runtime/tests/serialize_deserialize_test.js
index 70397cd..d418511 100644
--- a/core/runtime/tests/serialize_deserialize_test.js
+++ b/core/runtime/tests/serialize_deserialize_test.js
@@ -17,6 +17,40 @@ function assertArrayEquals(a1, a2) {
   }
 }
 
+function testIssue20727() {
+  // https://github.com/denoland/deno/issues/20727
+  const ab = new ArrayBuffer(10);
+  const transferList = [ab];
+  Deno.core.serialize(
+    { ab },
+    { transferredArrayBuffers: transferList },
+  );
+
+  // check that shared_array_buffer_store is set
+  assert(typeof transferList[0] === "number");
+  const data = {
+    array1: new Uint32Array([]),
+    array2: new Float32Array([]),
+  };
+
+  const transferListTwo = [
+    data.array1.buffer,
+    data.array2.buffer,
+  ];
+  const serializedMultipleTransferredBuffers = Deno.core.ops.op_serialize(
+    { id: 2, data },
+    { transferredArrayBuffers: transferListTwo },
+  );
+
+  assert(typeof transferListTwo[0] === "number");
+  assert(typeof transferListTwo[1] === "number");
+  // should not throw
+  Deno.core.ops.op_deserialize(
+    serializedMultipleTransferredBuffers,
+    { transferredArrayBuffers: transferListTwo },
+  );
+}
+
 function main() {
   const emptyString = "";
   const emptyStringSerialized = [255, 15, 34, 0];
@@ -69,6 +103,8 @@ function main() {
     new Uint8Array(circularObjectSerialized),
   );
   assert(deserializedCircularObject.test == deserializedCircularObject);
+
+  testIssue20727();
 }
 
 main();