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

Fix memory leak for Option<SwiftType> #273

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Fix memory leak for Option<SwiftType> #273

merged 1 commit into from
Apr 29, 2024

Conversation

chinedufn
Copy link
Owner

This commit fixes a memory leak when passing Option<SwiftType> to
extern "Rust" functions.

For example, the following bridge module would lead to memory leaks:

#[swift_bridge::bridge]
mod ffi {
    extern "Swift" {
        type SomeSwiftType;
    }

    extern "Rust" {
        fn rust_fn_takes_swift_type(arg: Option<SomeSwiftType>);
    }
}

When the above rust_fn_takes_swift_type function was called the
SomeSwiftType would be retained twice.

When the Rust side later freed SomeSwiftType the retain count would
drop from 2 to 1.

Since there was still 1 retain, SomeSwiftType would never get freed.

Impact

Support for passing Option<SomeSwiftType> from Swift to Rust was
introduced earlier today.

This functionality was released in swift-bridge = "0.1.54".

W will be deploying `swift-bridge = "0.1.55" today. "0.1.55" will
contain a fix for the leak.

Because support for Option<SomeSwiftType> was released today, and the
fix will be released today, it should be unlikely that anyone has
experienced this leak.

We will yank "0.1.54" from "crates.io".

All users (probably zero) that are bridging Option<SomeSwiftType> should
upgrade from "0.1.54" to "0.1.55" to.

Solution

This commit does the following:

  • When passing ownership of a Swift type from Swift to Rust we now
    increment the retain count by one. Before this commit we were
    incrementing it by two.

  • When passing ownership of a Swift type from Rust to Swift we avoid
    calling the the Rust handle's Drop implementation. We avoid this by
    using std::mem::forget.
    This ensures that the retain count does not get decremented.

  • When passing ownership of a Swift type from Rust to Swift the Swift
    side does not increment the retain count.

With all of the above we should now be able to:

  • Create an Option<SwiftType> Swift type on the Swift side

  • Pass ownership of the `Option to Rust. The retain count is now 1.

  • Pass ownership back to Swift. The retain count is still 1.

Before this commit, when passing an Option<SwiftType> it was:

  • Create a Swift type on the Swift side

  • Pass Option<SwiftType> to Rust. Retain count is now 2

  • Rust passes ownership back to Swift. Retain count is now 1

  • Retain count never hits 0. Memory has been leaked.

This commit fixes a memory leak when passing `Option<SwiftType>` to
`extern "Rust"` functions.

For example, the following bridge module would lead to memory leaks:

```rust
#[swift_bridge::bridge]
mod ffi {
    extern "Swift" {
        type SomeSwiftType;
    }

    extern "Rust" {
        fn rust_fn_takes_swift_type(arg: Option<SomeSwiftType>);
    }
}
```

When the above `rust_fn_takes_swift_type` function was called the
`SomeSwiftType` would be retained twice.

When the Rust side later freed `SomeSwiftType` the retain count would
drop from 2 to 1.

Since there was still 1 retain, `SomeSwiftType` would never get freed.

## Impact

Support for passing `Option<SomeSwiftType>` from Swift to Rust was
introduced earlier today.

This functionality was released in `swift-bridge = "0.1.54"`.

W will be deploying `swift-bridge = "0.1.55" today. "0.1.55" will
contain a fix for the leak.

Because support for `Option<SomeSwiftType>` was released today, and the
fix will be released today, it should be unlikely that anyone has
experienced this leak.

We will yank "0.1.54" from "crates.io".

All users (probably zero) that are bridging `Option<SomeSwiftType>` should
upgrade from "0.1.54" to "0.1.55" to.

## Solution

This commit does the following:

- When passing ownership of a Swift type from Swift to Rust we now
  increment the retain count by one. Before this commit we were
  incrementing it by two.

- When passing ownership of a Swift type from Rust to Swift we avoid
  calling the the Rust handle's `Drop` implementation. We avoid this by
  using `std::mem::forget`.
  This ensures that the retain count does not get decremented.

- When passing ownership of a Swift type from Rust to Swift the Swift
  side does not increment the retain count.

With all of the above we should now be able to:

- Create an `Option<SwiftType>` Swift type on the Swift side

- Pass ownership of the `Option<SwiftType> to Rust. The retain count is now 1.

- Pass ownership back to Swift. The retain count is still 1.

Before this commit, when passing an `Option<SwiftType>` it was:

- Create a Swift type on the Swift side

- Pass `Option<SwiftType>` to Rust. Retain count is now 2

- Rust passes ownership back to Swift. Retain count is now 1

- Retain count never hits 0. Memory has been leaked.
@@ -434,7 +444,7 @@ impl BridgeableType for OpaqueForeignType {
format!("{{ if let val = {expression} {{ val.isOwned = false; return val.ptr }} else {{ return nil }} }}()", expression = expression,)
}
HostLang::Swift => {
format!("{{ if let val = {expression} {{ return Unmanaged.passRetained(val).retain().toOpaque() }} else {{ return nil }} }}()")
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passRetained already increments the retain count. We do not want to increment it twice.

@chinedufn chinedufn merged commit 87dbea3 into master Apr 29, 2024
5 checks passed
@chinedufn chinedufn deleted the fix-mem-leak branch April 29, 2024 03:14
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.

None yet

1 participant