Object.lower() can result in use-after-free errors #1797

bendk · 2023-10-22T14:37:34Z

When an object is lowered from a foreign language we return the raw pointer from the foreign code then clone the Arc using the raw pointer in the Rust code.

How do we know that the pointer is still valid in that Rust code? For normal Rust calls, we can be sure since the a reference to the foreign object is still on the stack. However, for callbacks there are cases when this isn't true:

Thread A returns the raw pointer, then gets interrupted (after the Python function returns, but before the Rust code runs).
Thread B destroys the Rust object
Thread A continues and calls from_raw on the freed raw pointer.

I also think there's a related case when Rust returns a callback interface or trait interface.

The text was updated successfully, but these errors were encountered:

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.

bendk · 2023-10-31T20:59:20Z

I think the general system we should use is that object handles are passed as borrows and returned as owned values. In this case, we should be cloning the handle before returning it.

This seems pretty intuitive to me. In Rust you can pass a reference in without any special handling, but returning a reference requires lifetime checks. You can't check the lifetime of an object on the other side of the FFI, so the rule has to be any handle that's returned is an owned value.

bendk · 2023-11-01T13:57:15Z

I think the above comment represents the ideal solution, but implementing that at this point would be quite complex. We could split lower() into lower_arg() and lower_return() in the foreign bindings, like the Rust code does. But then what about container objects that get lowered into Rust buffers? I think we would need to split write into write_arg and write_return which just feels like too much work to me.

I'm going to go with the simpler solution: always clone the reference in lower(). It's an extra FFI call in some circumstances, but I think we can live with that.

I want to fix mozilla#1797 by cloning the object handle in lower(). However, this presents an issue in the dynamic languages because they perform various failable checks in lower(): type checks, bounds checks, etc. If the check fails, then the cloned handle will be leaked. Prevent this by adding a `check()` method. We do the check first then once we get to `lower()`, nothing can fail.

Currently, `lower()` always returns a borrow of the object handle. This is fine for function arguments, since you know the object is still alive on the stack while the function is being called. However, for function returns this is not correct. To fix this: clone the handle in `lower()`. Added a test for this -- it was surprisingly easy to cause a segfault with the current behavior.

Now object handles are always cloned before they're lowered, then consumed on the Rust side. This makes it so `lower` returns an owned handle instead of a borrowed one.