Skip to content

Commit 90913dd

Browse files
author
Sergey Bargamon
committed
found a way to get rid of libc
It turns out that using vec as a buffer works, and it is even more performant. And when rust-lang/rust#89292 will be merged, things will be even simpler (no pop from the buffer).
1 parent c9abef0 commit 90913dd

File tree

5 files changed

+56
-47
lines changed

5 files changed

+56
-47
lines changed

Cargo.lock

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ edition = "2018"
99
objc = "0.2"
1010
objc_id = "0.1"
1111
objc-foundation = "0.1"
12-
libc = "0.2"
1312

1413
[dev-dependencies]
1514
criterion = "0.3"

benches/my_benchmark.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
use criterion::{criterion_group, criterion_main, Criterion};
2-
use foundation::{leak, no_leak};
2+
use foundation::{leak, no_leak_vec};
33

44
fn criterion_benchmark(c: &mut Criterion) {
55
let mut group = c.benchmark_group("to string");
66
let str = &"aaa 🍺".repeat(100);
77
group.bench_with_input("old", str, |b, str| b.iter(|| leak(str)));
8-
group.bench_with_input("new", str, |b, str| b.iter(|| no_leak(str)));
8+
group.bench_with_input("new vec", str, |b, str| b.iter(|| no_leak_vec(str)));
99
group.finish()
1010
}
1111

readme.md

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
# This repo is about reproducible memory leak in objc_foundation`s INSstring.as_str().
22

3-
4-
53
## how to see a leak
64

75
```shell
@@ -21,7 +19,7 @@ pub fn leak(str: &str) {
2119

2220
## why it produces a leak?
2321

24-
INSString.as_str() internaly uses UTF8String property of NSString.
22+
INSString.as_str() internally uses UTF8String property of NSString.
2523
[Apple doc](https://developer.apple.com/documentation/foundation/nsstring/1411189-utf8string?language=objc)
2624
says that the memory behind this pointer has a lifetime shorter than a lifetime of an NSString itself.
2725
But apparently, this is not entirely true. At least, this statement is not valid for strings that contain
@@ -36,27 +34,32 @@ So in the end, the actual leak occurs not in INSString.as_str() but, I guess, in
3634
Yes. NSString::getCString ([Apple doc](https://developer.apple.com/documentation/foundation/nsstring/1415702-getcstring)) and we can use it like this:
3735

3836
```rust
39-
pub fn nsstring_to_rust_string(nsstring: Id<NSString>) -> String {
40-
unsafe {
41-
let string_size: usize = msg_send![nsstring, lengthOfBytesUsingEncoding: 4];
42-
// + 1 is because getCString returns null terminated string
43-
let buffer = libc::malloc(string_size + 1) as *mut c_char;
44-
let is_success: bool = msg_send![nsstring, getCString:buffer maxLength:string_size+1 encoding:4];
45-
if is_success {
46-
// CString will take care of memory from now on
47-
CString::from_raw(buffer).to_str().unwrap().to_owned()
48-
} else {
49-
// In case getCString failed there is no point in creating CString
50-
// So we must free memory
51-
libc::free(buffer as *mut c_void);
52-
// Original NSString::as_str() swallows all the errors.
53-
// Not sure if that is the correct approach, but we also don`t have errors here.
54-
"".to_string()
37+
pub fn convert_with_vec(nsstring: Id<NSString>) -> String {
38+
let string_size: usize = unsafe { msg_send![nsstring, lengthOfBytesUsingEncoding: 4] };
39+
let mut buffer: Vec<u8> = vec![0_u8; string_size + 1];
40+
let is_success: bool = unsafe {
41+
msg_send![nsstring, getCString:buffer.as_mut_ptr() maxLength:string_size+1 encoding:4]
42+
};
43+
if is_success {
44+
// before from_vec_with_nul can be used https://github.com/rust-lang/rust/pull/89292
45+
// nul termination from the buffer should be removed by hands
46+
buffer.pop();
47+
48+
unsafe {
49+
CString::from_vec_unchecked(buffer)
50+
.to_str()
51+
.unwrap()
52+
.to_string()
5553
}
54+
} else {
55+
// In case getCString failed there is no point in creating CString
56+
// Original NSString::as_str() swallows all the errors.
57+
// Not sure if that is the correct approach, but we also don`t have errors here.
58+
"".to_string()
5659
}
5760
}
5861
```
59-
If you change in main.rs leak to no_leak and again will run again
62+
If you change in main.rs leak to no_leak_vec and will run again
6063
```shell
6164
cargo instruments -t Allocations
6265
```
@@ -67,6 +70,11 @@ cargo becnh
6770
```
6871
You will see that performance even better.
6972

73+
```shell
74+
to string/old time: [12.855 us 12.961 us 13.071 us]
75+
to string/new vec time: [10.477 us 10.586 us 10.699 us]
76+
```
77+
7078

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

src/lib.rs

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,16 @@
11
use objc::{msg_send, sel, sel_impl};
22
use objc_foundation::{INSString, NSString};
33
use objc_id::Id;
4-
use std::ffi::{c_void, CString};
5-
use std::os::raw::c_char;
4+
use std::ffi::CString;
65

76
pub fn leak(str: &str) {
87
let nsstrig = NSString::from_str(str);
98
nsstrig.as_str();
109
}
1110

12-
pub fn no_leak(str: &str) {
11+
pub fn no_leak_vec(str: &str) {
1312
let nsstrig = NSString::from_str(str);
14-
nsstring_to_rust_string(nsstrig);
13+
convert_with_vec(nsstrig);
1514
}
1615

1716
/**
@@ -26,24 +25,28 @@ getCString:
2625
https://developer.apple.com/documentation/foundation/nsstring/1415702-getcstring
2726
2827
*/
29-
pub fn nsstring_to_rust_string(nsstring: Id<NSString>) -> String {
30-
unsafe {
31-
let string_size: usize = msg_send![nsstring, lengthOfBytesUsingEncoding: 4];
32-
// + 1 is because getCString returns null terminated string
33-
let buffer = libc::malloc(string_size + 1) as *mut c_char;
34-
let is_success: bool =
35-
msg_send![nsstring, getCString:buffer maxLength:string_size+1 encoding:4];
36-
if is_success {
37-
// CString will take care of memory from now on
38-
CString::from_raw(buffer).to_str().unwrap().to_owned()
39-
} else {
40-
// In case getCString failed there is no point in creating CString
41-
// So we must free memory
42-
libc::free(buffer as *mut c_void);
43-
// Original NSString::as_str() swallows all the errors.
44-
// Not sure if that is the correct approach, but we also don`t have errors here.
45-
"".to_string()
28+
pub fn convert_with_vec(nsstring: Id<NSString>) -> String {
29+
let string_size: usize = unsafe { msg_send![nsstring, lengthOfBytesUsingEncoding: 4] };
30+
let mut buffer: Vec<u8> = vec![0_u8; string_size + 1];
31+
let is_success: bool = unsafe {
32+
msg_send![nsstring, getCString:buffer.as_mut_ptr() maxLength:string_size+1 encoding:4]
33+
};
34+
if is_success {
35+
// before from_vec_with_nul can be used https://github.com/rust-lang/rust/pull/89292
36+
// nul termination from the buffer should be removed by hands
37+
buffer.pop();
38+
39+
unsafe {
40+
CString::from_vec_unchecked(buffer)
41+
.to_str()
42+
.unwrap()
43+
.to_string()
4644
}
45+
} else {
46+
// In case getCString failed there is no point in creating CString
47+
// Original NSString::as_str() swallows all the errors.
48+
// Not sure if that is the correct approach, but we also don`t have errors here.
49+
"".to_string()
4750
}
4851
}
4952

@@ -53,7 +56,7 @@ mod tests {
5356

5457
#[test]
5558
fn it_works() {
56-
let nsstrig = NSString::from_str("aaabbb🍺Ыض");
57-
assert_eq!(nsstring_to_rust_string(nsstrig), "aaabbb🍺Ыض");
59+
let text = "aaabbb🍺Ыض";
60+
assert_eq!(convert_with_vec(NSString::from_str(text)), text);
5861
}
5962
}

0 commit comments

Comments
 (0)