bug: critical DOUBLE-FREE problem for all calls to post()

[fzyzcjy]

Please correct me if I am wrong. Suppose we call post with some vec of u8.

Looking at

https://github.com/sunshine-protocol/allo-isolate/blob/d10c582aba2a41f65889f1270f009b4c9a4c899b/src/lib.rs#L117-L125
the logic is:

• create raw pointer of that DartCObject (containing a big array)
• call Dart_PostCObject function
• use Rust's logic to free the Box of that DartCObject

When drop(boxed_obj) it will call the following:

https://github.com/sunshine-protocol/allo-isolate/blob/d10c582aba2a41f65889f1270f009b4c9a4c899b/src/ffi.rs#L152-L184
So, since it is a DartTypedData, will call let _ = unsafe { Vec::from_raw_parts(...) }. Then when the _ variable leaves the scope, Rust will drop that Vec. In other words, the big array in memory is dropped (once) here.

However, on the other hand, consider what Dart_PostCObject says: dart-lang/sdk#47270 (comment)

• If true is returned, the message was enqueued, and finalizers for external
• typed data will eventually run, even if the receiving isolate shuts down
• before processing the message. If false is returned, the message was not
• enqueued and ownership of external typed data in the message remains with the
• caller.

To begin with, there do definitely exist a possible issue here: #6

However, more critical issue is double free. Notice that, the callback tied with typed data will finally be called (if Dart_PostCObject returns true - which is most of the cases). And we do https://github.com/sunshine-protocol/allo-isolate/blob/d10c582aba2a41f65889f1270f009b4c9a4c899b/src/into_dart.rs#L205 . In other words, we call deallocate_rust_buffer which is drop(Vec::from_raw_parts(ptr, len, len));. Hey, this is another free of the big array!

So we have double free! Dangerous!

Possibly related: #3
@jerel @shekohex

[shekohex]

Hello, thanks for the write-up! it is really great!

The only part I guess you got a bit confused about: that's there is two different types/variant inside the DartCObject

1. DartNativeTypedData which is as you described above, but the only missing thing about it here is that for DartNativeTypedData it is get copied to the dartvm memory, so the ownership is never transferred (afaik).
2. DartNativeExternalTypedData which is a bit different than the above one, as in this case, the VM does not really copy the memory (it borrows the data) and when it is done, it calls the callback to free the memory.

So, unless you wrap your buffer the Vec<u8> in a ZeroCopyBuffer it will use the above drop method implemented for DartCObject, otherwise it will use the callback inside the DartNativeTypedData.

Hope that was clear! as that's far as I understand, from reading some internal code of the dartvm.

[jerel]

Also, on item 2 above the buffer is forgotten by the use of ManuallyDrop https://github.com/sunshine-protocol/allo-isolate/blob/d10c582aba2a41f65889f1270f009b4c9a4c899b/src/into_dart.rs#L191 so the expectation is that while the outer DartCObject can be cleaned up by drop in lib.rs the shared buffer it contained will be freed later by a call from the Dart finalizer to deallocate_rust_buffer.

[fzyzcjy]

I think you are right! The DartNativeTypedData has different implementations compared with DartNativeExternalTypedData and I get confused.

Maybe I can make a PR, such that we try to refactor the code to put the relevant code together? Then the code can be easier to understand and thus maintain.

[shekohex]

PRs are always welcome!
Thank you.