memory leak in INSstring.as_str()

[Lurk]

how to reproduce

pub fn leak() {
    let nsstrig = NSString::from_str("aaabbb");
    nsstrig.as_str(); 
}

repo with examples and tests https://github.com/Lurk/nsstring_leak

why it is leaking

INSString.as_str() internaly uses UTF8String property of NSString. Apple doc says that the memory behind this pointer has a lifetime shorter than a lifetime of an NSString itself. But apparently, this is not entirely true. At least, this statement is not valid for strings that contain characters outside the ASCI range. And sometimes for strings that do not.

Sadly, I did not find any reason for that.

So, in the end, the actual leak occurs not in INSString.as_str() but, I guess, in objc runtime.

Is there a workaround?

Yes. NSString::getCString (Apple doc) and we can use it like this:

pub fn nsstring_to_rust_string(nsstring: Id<NSString>) -> String {
    unsafe {
        let string_size: usize = msg_send![nsstring, lengthOfBytesUsingEncoding: 4];
        // + 1 is because getCString returns null terminated string
        let buffer = libc::malloc(string_size + 1) as *mut c_char;
        let is_success: bool = msg_send![nsstring, getCString:buffer  maxLength:string_size+1 encoding:4];
        if is_success {
            // CString will take care of memory from now on
            CString::from_raw(buffer).to_str().unwrap().to_owned()
        } else {
            // In case getCString failed there is no point in creating CString
            // So we must free memory
            libc::free(buffer as *mut c_void);
            // Original NSString::as_str() swallows all the errors.
            // Not sure if that is the correct approach, but we also don`t have errors here.
            "".to_string()
        }
    }
}

In the repo I mentioned above, you will find a test and benchmark for that.

The only problem I see with this solution is that it has a different return type (String instead of &str).
If you know how to fix that, or any other idea on how to do things better - please let me know.

[Lurk]

Found a way to get rid of libc::malloc and libc::free

pub fn convert_with_vec(nsstring: Id<NSString>) -> String {
    let string_size: usize = unsafe { msg_send![nsstring, lengthOfBytesUsingEncoding: 4] };
    let mut buffer: Vec<u8> = vec![0_u8; string_size + 1];
    let is_success: bool = unsafe {
        msg_send![nsstring, getCString:buffer.as_mut_ptr()  maxLength:string_size+1 encoding:4]
    };
    if is_success {
        // before from_vec_with_nul can be used https://github.com/rust-lang/rust/pull/89292
        // nul termination from the buffer should be removed by hands
        buffer.pop();

        unsafe {
            CString::from_vec_unchecked(buffer)
                .to_str()
                .unwrap()
                .to_string()
        }
    } else {
        // In case getCString failed there is no point in creating CString
        // Original NSString::as_str() swallows all the errors.
        // Not sure if that is the correct approach, but we also don`t have errors here.
        "".to_string()
    }
}

updated repo with examples and benchmarks https://github.com/Lurk/nsstring_leak

[madsmtm]

Pretty sure you can fix the memory leak with:

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

autoreleasepool(|| {
    let s = NSString::from_str("aaabbb");
    s.as_str();
});

See my explanation here: SSheldon/rust-objc#103 (comment) (this pull request would also be the way to fix this problem, because we would change the signature of NSString::as_str to require an AutoreleasePool reference).

[Lurk]

@madsmtm thank you, with autoreleasepool leak is gone.

Do you have any idea why getCString might be slightly faster than UTF8String?
My benchmark shows those numbers:

to string/old           time:   [12.551 us 12.644 us 12.733 us]
to string/autorelease   time:   [12.775 us 12.998 us 13.207 us]
to string/vec           time:   [10.107 us 10.208 us 10.307 us]    

[madsmtm]

Haven't looked much at the performance of these things, but a quick guess would be that UTF8String has to do more work; it would at the very least have:

• An extra check for whether it has to allocate or not
• Some code to autorelease the allocated buffer
  But at these speeds (and when we don't have the source code for it) it's hard to tell.

Oh, and btw, and just plugging 4 in as the encoding is UB, integers are i32 by default while the expected parameter is NSStringEncoding (typedef of NSUInteger, roughly usize). You should define a variable/constant with an explicit type or use 4usize, otherwise msg_send! will infer the wrong type!