Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slabs and handles #1808

Closed
wants to merge 6 commits into from
Closed

Slabs and handles #1808

wants to merge 6 commits into from

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Oct 26, 2023

Motivation

#1730 discusses this on more general grounds, but there's also a lot of the practical motivation.

When I was doing the async work, I often had to struggle with the issue of how to pass a new object type across the FFI (the future callback data, the continuation data, the foreign executor, etc.). Each time I wanted a new object type, I needed to:

  • Figure out how to make the into_raw() / from_raw() calls correctly.
  • Add an FfiType variant and add a bunch of unimplemented!() stubs
  • Figure out how to actually handle the new type in the foreign languages. Here Swift was always the worst: Should it be UnsafeRawPointer or UnsafeRawMutablePointer? How should it be rendered in the bridging header?

Going the other way, passing a foreign object back to Rust, was just as tedious if not more so. Here Swift wasn't that bad and Kotlin was the worst, since JNA doesn't give you a way to leak a pointer.

This work was tedious and felt dangerous. I regularly saw segfaults from seemingly innocent code.

Passing objects using handles is a pretty fundamental thing, we should take steps to get it right.

Slabs and handles

The solution I came up with was to use a data structure similar to the tokio slab crate, with some additions to make it thread-safe etc. See the first commit for details.

I went with a hand-written solution because wanted this to be reasonable fast, tailored to our needs, and not force users to take on a large dependency. I think it's okay to accept the extra complexity, mostly because I have faith in the loom tests after they found so many bugs in my code.

However, I would be okay with picking something else. Maybe we could use sharded-slab, chashmap, or just Rwlock<HashMap<u32, T>>. My main goal is to get the other commits merged.

The other commits

The other commits mostly rename things and rewrite comments, but there are some nice wins scattered in there and I want to call out a couple:

@bendk bendk requested a review from mhammond October 26, 2023 22:13
@bendk bendk requested a review from a team as a code owner October 26, 2023 22:13
@bendk
Copy link
Contributor Author

bendk commented Oct 26, 2023

@arg0d I'd love any feedback on this one from an external bindings author. In the short-term, this would force you to update some of your FFI code, but hopefully would simplify things in the long-term.

@bendk bendk force-pushed the slabs-and-handles branch 2 times, most recently from 4d29aac to 0879f72 Compare October 27, 2023 00:17
@arg0d
Copy link
Contributor

arg0d commented Oct 27, 2023

I'm failling to grasp the problem this PR solves. Passing callback handles and data for future callbacks from foreign to Rust doesn't seem like that difficult of a problem. There are 2 main ways to do that:

  • use utilities provided by the language to create handles to objects
  • roll your own custom handle map solution

C# provides GCHandle with explicit Alloc and Free methods to control the lifetime of the handle. It also provides some options to control the lifetime of the object, i.e. you can specify that it should be a weak reference, so I'm assuming the object may be collected by GC, but GCHandle remains valid and returns null when trying to dereference it.

Go is simillar to C#, and provides cgo.Handle with explicit cgo.NewHandle and cgo.Handle.Delete functions to control the lifetime of the handle.

From my limited testing, both C# and Go Handles work in a defined manner when misused. Attempting to double free or use a handle after freeing it results in C# exceptions and Go panics. Entire Go ecosystem is based on an idea that all operations are defined, and I guess the same applies to Handle. I'm not sure about C#, because MSDN mentions in remarks that The caller must ensure that for a given handle is called only once., but doesn't mention anything about whether its guaranteed that exceptions will be thrown.

Rolling your own handle map seems to be quite trivial. The only consideration is synchronization, but I believe most of languages supported by uniffi provide convenient access to synchronization primitives. The simplicity of writing your own handle map results in very predictable and defined behaviour. With custom handle map, its easy to detect use after free, and you decide what happens when a freed handle is dereferenced. In this regard, custom handle map seems like a safer option than using system handles. Its unclear if system handles behave in a defined manner in all cases. For example, how can we guarantee that an exception is thrown when accessing a GCHandle on another random thread, and after millions of other GCHandle objects have been cycled by the application? The docs aren't very reassuring, so the only other option would be to inspect the implementation of GCHandle, and hope its the same for past and future versions.

Considering the above options, I guess what I'm most confused about is whats the point of creating a counterpart handle on Rust side for each foreign language handle. I understand the use case for ref counting object handles, but that seems like a completely different handles use case from callbacks. I agree that object handles need some improvement, but I don't think that adding more complexity solves the problem.

For objects, it sounds like a better solution would to implement use-after-free and double-free protection in object FFI function level, i.e. using freed handles with FFI object functions is defined behaviour, and in such cases FFI functions return a special error code. This would add performance penalty, because each object access would require a liveliness check on Rust side. As I understand this is exactly what is happening in this PR, albeit the liveness check happens outside of object FFI function. I think adding the same liveliness checks like in this PR, but directly to object FFI functions (methods and free) would result in more robust foreign language bindings, since the bindings wouldn't need to worry at all about freed memory access.

@arg0d
Copy link
Contributor

arg0d commented Oct 27, 2023

Adding some links:

@bendk
Copy link
Contributor Author

bendk commented Oct 27, 2023

It sounds like both C# and Go have some nice ways to create usized-handles, I really want to move away from that approach though because it's not so easy in Kotlin/Swift/Python and also because I want to access that FOREIGN_BIT, but I'm afraid to make any assumptions about the bit pattern of thoes values.

Using a handle map would work fine. The simplicity of it all is definitely appealing. You get the use-after-free protection, since you can just keep incrementing the handle counter. I guess it's possible for the counter to roll over, but I don't think the detection needs to be 100% bullet-proof. The slab approach does have a couple of benefits though:

  • The inc-ref is nice for foreign objects that implement trait interfaces. You could accomplish the same thing with a clone method that returned a new handle, but I'm pretty sure you'd take a performance hit. You're right that it's not needed for callbacks or continuation data, but I'd rather have one API that works for everything.
  • It avoids the read-write lock for the first part of the operation. There's something that feels bad to me about having an insert/remove block a read. That said, most of the languages still use a read-write lock for the second part, when they're accessing the underlying array.
  • Rust and foreign languages share the same handle API? I'm not sure if this is really that important. I also wouldn't be against using a handle map on Rust.

I lean slightly towards this approach, but it would not be hard to convince me to go with a handle map. My main goal is to decide on one system for everything and to have that system be based on ints rather than pointers/usize.

I didn't quite get that last paragraph. The use-after-free checks are currently integrated into the method FFI calls. There's not a special code, it just uses the unexpected error code with a message saying something like "did you use the handle after it was freed?". It's a bit weird how this happens in Rust, since the function panics, then is caught by catch_unwind. I think this could be improved, but this PR is already too big.

@arg0d
Copy link
Contributor

arg0d commented Oct 27, 2023

I seem to be behind some terminology. What are trait interfaces? Are these somehow different from objects/interfaces?

The use-after-free checks are currently integrated into the method FFI calls.

Where exactly are these intergrated? From what I understand the entire reason for having ref counting and checking for use-after free in ObjectRuntime.kt is to ensure Kotlin does not access freed Rust memory. Also, if use after free checks are already implemented on Rust side, then whats the point of implementing this new slab code in foreign bindings? I have just tried to disable these checks in Kotlin code, and ran modified Sprites Kotlin test. The test passes with flying colors, no errors are reported. Whats more interesting is that when I tried to simplify the test to paste in here, the test started hanging after trying to use the freed object.

import uniffi.sprites.*;

val sempty = Sprite(null)
assert( sempty.getPosition() == Point(0.0, 0.0) )

val s = Sprite(Point(0.0, 1.0))
assert( s.getPosition() == Point(0.0, 1.0) )

s.moveTo(Point(1.0, 2.0))
assert( s.getPosition() == Point(1.0, 2.0) )

s.moveBy(Vector(-4.0, 2.0))
s.destroy()
s.destroy()
s.destroy()
s.destroy()
assert( s.getPosition() == Point(-3.0, 4.0) )

// s.destroy()
// try {
//     s.moveBy(Vector(0.0, 0.0))
//     assert(false) { "Should not be able to call anything after `destroy`" }
// } catch(e: IllegalStateException) {
//     assert(true)
// }

val srel = Sprite.newRelativeTo(Point(0.0, 1.0), Vector(1.0, 1.5))
assert( srel.getPosition() == Point(1.0, 2.5) )

@bendk
Copy link
Contributor Author

bendk commented Oct 27, 2023

I seem to be behind some terminology. What are trait interfaces? Are these somehow different from objects/interfaces?

It's a new feature. In 0.25 it allows you to pass an Arc<dyn Trait> over the FFI rather than Arc<T>. In 0.26 it will also allow the foreign side to implement the trait and pass it across the FFI.

It's related to this PR because it's useful to have a bit available in the handle for distinguishing a foreign implementation vs a Rust implementation.

The use-after-free checks are currently integrated into the method FFI calls.

Where exactly are these integrated?

I think I'm getting your point better now, but still don't fully understand. Hopefully this makes sense, sorry if it doesn't.

When Rust tries to lift an object it calls SlabAlloc::remove, which calls Slab::remove_or_panic. This all happens inside a std::panic::catch_unwind call. So if we detect a use-after-free, this should end up as an unexpected error thrown on the foreign side. Method receivers are a special case of this.

From what I understand the entire reason for having ref counting and checking for use-after free in ObjectRuntime.kt is to ensure Kotlin does not access freed Rust memory.

Yes the current object code has checks on the foreign side. They're mostly working okay, but I believe there's a corner case where the foreign side can't properly hold a reference (#1797). I also like the idea of having the check in Rust and not having to implement it in each foreign language.

Also, if use after free checks are already implemented on Rust side, then whats the point of implementing this new slab code in foreign bindings?

This is for when the you want to pass an object from the foreign side to Rust. Right now this is callback interfaces, continuation data. I'm also hoping to use it for foreign executors.

@bendk bendk force-pushed the slabs-and-handles branch 2 times, most recently from 391a6bc to 98350a0 Compare October 27, 2023 17:38
This will be used for passing handles across the FFI.  This have several
advantages as an FFI type:

* Generation counter to detect use-after-free bugs
* Slab ID to detect using a handle with the wrong type.
* The same data structures can be used on the foreign side, rather than us
  having to figure out how to leak references in all languages.
* Integers come with less gotchas.  For example, we use a bit to
  differentiate between foreign and Rust handles.  This would be
  possible with tagged pointers but there's a lot of details to worry
  about there.  See the `tagged_pointer` crate.
* Constant width at 64 bits rather than using the platform word size.
  This will simplify some things, especially reading/writing them to
  `RustBuffer`
* Only the first 48 bits are significant which helps with languages like JS.

Performance should be pretty good.  `get` and `inc_ref` are basically
lock-free thanks to the `append_only_vec` crate and some atomic code.
`insert` and `remove` that frees an entry use a shared lock. We could
speed that up by keeping thread-local free lists, but that seems like
overkill since the lock should rarely be contended.

* For objects, this represents a small overhead over simply leaking the
  Arc.  The same is true for the Swift objects that we leak using
  `Unmanaged<>`.
* For trait interfaces, this is probably a small gain compared to adding
  an extra box, then leaking it.
* This is going to be way faster than the foreign code that uses a lock
  and a map.

As mentioned above, I'm pretty sure that we can leverage this for
foreign handles as well, and should be able to remove some code on from
the bindings.
* Added the `SlabAlloc` FFI trait.  This is used to manage handles for
  `Arc<T>` instances, including `Arc<dyn Trait>`
* Also use handles for trait interfaces.  This still needs some
  updates on the foreign side before it's working.
* Renamed a bunch of stuff and replaced a lot of comment text
* Added the `const_random` crate to generate random slab IDs.  This
  should be a pretty lightweight dpeendency.
* Bumped `CONTRACT_VERSION` since this is a change to the FFI
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.
Exported Rust FFI functions to manage a Slab that stores unit values.
The foreign code uses this to allocate and manage handles then stores the
actual data inside a native array.

This replaces the previous handle maps / pointer leaking code for
callbacks, trait interfaces and continuation data.

Started updating the ForeignExecutor code to use handles, but this is
still a WIP while the ForeignExecutor type is in it's limbo state.
Check if the trait interface handle originates from the other side of
the FFI.  If it does, inc-ref it and return the handle, rather than
returning a handle to the wrapped object.  Before, each time the trait
object was passed across the FFI, we wrapped it another time

On the foreign side, this can be accomplished by a type check.  On the
Rust side, this requires an extra, `#[doc(hidden)]` method on the trait.
This means that UDL-defined trait interfaces need to be wrapped with an
attribute macro that inserts that method.

One issue with this is that it can cause us to leak references if we do
the inc-ref, then there's an exception lowering another argument.  I
believe this can be fixed by separating out the failable lowering code
from the non-failable code.
The dynamic languages performed various checks in lower(): type checks, bounds
checks, etc.  This presented an issue when we wanted to call `inc_ref`
for object types.  If we did this, then another value failed its check,
we would leak the reference.

Prevent this by adding a `check()` method.  We do the check first then
once we get to `lower()`, nothing can fail.
@arg0d
Copy link
Contributor

arg0d commented Oct 31, 2023

I see, so a trait interface basically makes objects interchangeable with callbacks? In that case I see where you are coming from. Sharing handle map state between Rust and foreign bindings would allow to short circuit dispatch straight into foreign trait implementation, without having to dispatch into Rust. The bit you are talking about could be used to determine if the trait originates from foreign language. Vice versa is also possible with Rust.

I think its worth to mention that this is only necessary in quite specific setups. The trait object has to make a round trip between Rust/foreign bindings in order for this short circuiting to make sense. If the round trip doesn't happen, then there is no short circuiting to be done. For example, it seems like in #1815 the roundtrip does not happen. All trait implementations would exist just in foreign side. This seems like a very niche use case. Have you got an actual use case for this? Example of a trait roundtrip:

class ForeignTrait: RustTraitInterface { .. }
returnTrait(ForeignTrait()).helloWorld()

My two cents about this PR: I'm not against this PR, I just don't agree about foreign language handle map with Rust handle map. I think these should be 2 separate unrelated concepts. Joining them into a single system adds more complexity, and I don't really see the benefit of this complexity. The indirection and synchronization this PR adds for objects doesn't directly require sharing handle state between Rust and foreign bindings. For example, for objects, the handle lookup from slab could just as easily be implemented in object FFI Rust function. This would take the load of foreign bindings to ensure Rust memory is not accessed after freeing it.

@bendk
Copy link
Contributor Author

bendk commented Oct 31, 2023

My two cents about this PR: I'm not against this PR, I just don't agree about foreign language handle map with Rust handle map. I think these should be 2 separate unrelated concepts. Joining them into a single system adds more complexity, and I don't really see the benefit of this complexity. The indirection and synchronization this PR adds for objects doesn't directly require sharing handle state between Rust and foreign bindings.

Yeah, this makes sense to me. Seems like the added complexity is not worth it. I'm going to close this one and try again starting from the simplest/least controversial parts and leave out any major changes to the foreign side.

The trait object has to make a round trip between Rust/foreign bindings in order for this short circuiting to make sense. (...) Have you got an actual use case for this?

The use case we've been thinking about is sync engines in Firefox. Currently we have a bunch of individual sync engines written in Rust and a Rust sync manager that uses them. We want to also allow the foreign code to implement their own sync engines. In that scenario, I could see round-tripping being pretty common. For example:

rust_sync_manager.start_sync([
   RustLoginsEngine(),
   RustPlacesEngine(),
   PythonTabsEngine(),
])

In that scenario, all the rust engines are round-tripping through the foreign code.

Thanks for the review, I really appreciate it.

@bendk bendk closed this Oct 31, 2023
@arg0d
Copy link
Contributor

arg0d commented Oct 31, 2023

I see, your use case seems very flexible :) I think the short circuiting could still be accomplished without sharing handle state between foreign bindings and Rust.

First scenario.

  • Rust domain inserts a new handle into Rust's handle map when returning RustLoginsEngine.
  • In start_sync, Rust receives a handle that already exists in Rust's handle map.
  • The dispatch can be short circuited.

Second scenario.

  • Kotlin instantiates new trait interface, inserts new handle into Kotlin's handle map.
  • Kotlin passes the handle into start_sync.
  • In start_sync, Rust receives a handle that does not exist in Rust's handle map.
  • The dispatch is not supposed to be short circuited.

Third scenario.

  • Kotlin instantiates new trait interface, inserts new handle into Kotlin's handle map.
  • Kotlin roundtrips the handle in some way.
  • After roundtrip, Kotlin receives a handle that exists in Kotlin's handle map.
  • The dispatch can be short circuited.

This sounds like it could work, but I see a problem in here. When each domain (Rust or foreign) works with a handle, if the handle does not exist in their respective handle map, a domain must assume that the handle belongs to the other domain and dispatch there. This would make it difficult to detect errors when a non existant handle is being used. Each domain would indefinitely dispatch to one another, since neither have a live handle.

@bendk
Copy link
Contributor Author

bendk commented Oct 31, 2023

The other side of that coin is if the same handle value exists in both maps and therefore both sides think it's theirs.

Either way, I think the requirement has to be that we can determine which side owns a handle by inspecting the raw value. This basically just means we need an unused bit that we can set on one side and not on the other. I'm going to make a PR that does that without so many other changes.

I should add that I never thought that both sides needed to share state. The foreignslab.rs module kind of did that, but the motivation there was code re-use.

@bendk bendk mentioned this pull request Nov 1, 2023
@bendk bendk mentioned this pull request Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants