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

Make autoreleasepool take the pool as a parameter #103

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link

@madsmtm madsmtm commented Jun 13, 2021

The pool is given as a reference, and the lifetime of the reference is the same as the lifetime of autoreleased objects in the pool. This allows us to bound the lifetimes of autoreleased objects, thereby making them safe to return.

Extracted from #98, see #95 for some more examples and the reasoning behind this.

Unsoundness Fixed.

This is unsound until we can prevent people from doing something like:

autoreleasepool(|outer_pool| {
    let obj = autoreleasepool(|inner_pool| {
        let obj = needs_lifetime_from_pool(outer_pool);
        obj
    });
    // obj can wrongly be used here because it's lifetime was assigned to the outer pool, but it has been released by the inner pool already
});

I think this is possible to statically prevent using the nightly auto_traits + negative_impls features. Done!

It might also be possible to make a stable implementation for debug_assertions builds that panic if it happens. Done!

The lifetime of the parameter is the lifetime of references in the pool. This allows us to bound lifetimes on autoreleased objects, thereby making them safe to return.
A nightly-only feature that adds the auto trait `AutoreleaseSafe` and uses it to prevent calling `autoreleasepool` with closures that capture an outer `AutoreleasePool`.

This fixes on nightly the unsoundness hole preventing `autoreleasepool` from being used to safely construct references to autoreleased objects.
Prevent using the lifetime from an outer pool when an inner pool is active.
@SSheldon
Copy link
Owner

SSheldon commented Aug 4, 2021

This is a lot of complexity for a nightly-only feature to allow safer autoreleases. I'm not sure why we need autoreleases to be safer. I'd say in rust people should only be using retain and release. Regardless, I'm not sure this would be worthwhile until the necessary features stabilize.

@SSheldon
Copy link
Owner

SSheldon commented Aug 4, 2021

To elaborate on this:

I'm not sure why we need autoreleases to be safer. I'd say in rust people should only be using retain and release.

My understanding of autorelease is that it's predominantly used when returning an object so that it's the caller's responsibility to keep the object alive as long as it needs; you won't return them a +1 retain count object that they forget to release and now you've caused a memory leak in their code. I don't see how this problem applies to rust with its ownership system: I can return a smart pointer that will release the object when the caller is done with it and it drops. This seems like a strictly better and more idiomatic solution in rust, so I don't see why safe rust code should be using autorelease.

@madsmtm
Copy link
Author

madsmtm commented Aug 6, 2021

The primary motivator for this is to make a safe function using NSString's UTF8String that can return a &str without (external) cloning.

The documentation on UTF8String is a bit sparse and difficult to grok, but this is what I found with educated guesses and testing: NSString stores the equivalent of an NSData internally, sometimes with an UTF-8 encoding, sometimes in other encodings (UTF-16?).
When you send it UTF8String it checks the internal encoding:

  • If the data is UTF-8 encoded, it returns a pointer into the internal NSData.
  • If the data is in another encoding, it creates a new NSData and writes the UTF-8 representation of the string into it, autoreleases the NSData, and returns a pointer to that NSData.

What this means is that the lifetime of the returned pointer is either the same as the NSString OR the lifetime of the innermost @autoreleasepool. Since the pointer is not an object but rather an inner field in some object, we can't just retain the pointer. Hence we need the ability to describe the lifetime of the innermost @autoreleasepool, and that's exactly what this PR enables you to do.

For reference, objc-foundation's implementation is currently unsound on this point, it would break on the following:

extern crate objc;
extern crate objc_foundation;

use objc::rc::autoreleasepool;
use objc_foundation::{NSString, INSObject, INSString};

fn main() {
    let nsstring = NSString::from_str("String with non-ascii characters: 🔨");
    let s = autoreleasepool(|| {
        nsstring.as_str()
    });
    // `s` is invalid here
    println!("{}", s);
}

Running this with Guard Malloc (environment variable: DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib) unearths the segmentation fault consistently.

Also, fruity's implementation marks the function as unsafe for exactly this reason.

Secondary motivator is to be able to return autoreleased references for functions that inherently does that, like NSString's +stringWithUTF8String, NSMenuItem's +separatorItem, and many others without having to retain and release these objects unnecessarily!

@madsmtm
Copy link
Author

madsmtm commented Aug 6, 2021

A simpler (but also more restricting) version than all the thread local stuff in debug mode could also just be to use some other unsafe auto trait that AutoreleasePool isn't, like Sync or Send:

// Nightly w. feature
pub fn autoreleasepool<'p, T>(f: impl FnOnce(&'p AutoreleasePool) -> T + AutoreleaseSafe) { ... }
// Stable
pub fn autoreleasepool<'p, T>(f: impl FnOnce(&'p AutoreleasePool) -> T + Sync) { ... } // Or `Send`

madsmtm added a commit to madsmtm/objc2 that referenced this pull request Oct 30, 2021
See the code comments and my explanation here: SSheldon/rust-objc#103 (comment)

This also has the nice "side-effect" of fixing the memory leaks that as_str was otherwise exhibiting when using non-ascii strings, see SSheldon/rust-objc-foundation#15.
@madsmtm madsmtm mentioned this pull request May 7, 2023
3 tasks
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