transpile: Do not leak main argument strings
#1447
Conversation
Rua
force-pushed
the
main-leak
branch
3 times, most recently
from
e309b65 to
e10b712
Compare
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Can you add a snapshot test for this, too?
Rua
force-pushed
the
main-leak
branch
2 times, most recently
from
326ac44 to
ba0f652
Compare
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Sorry, can you also do the thing where the snapshot test is added first before the fix, then updated along with the fix? Thanks!
| let mut args_strings: Vec<Vec<u8>> = ::std::env::args() | ||
| .map(|arg| { | ||
| ::std::ffi::CString::new(arg) | ||
| .expect("Failed to convert argument into CString.") | ||
| .into_bytes_with_nul() | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
I think it's better to store a Vec<CString> here. It's clearer what it's supposed to represent that way, and we can call CString::raw on it (c_string.into_raw().cast::<c_char>()).
There was a problem hiding this comment.
But that would reintroduce the leak. I used Vec<u8> because it lets you get a mutable reference without leaking, which CString does not.
There was a problem hiding this comment.
How exactly? I might've not been clear enough. I'm suggesting we store args_strings: Vec<CString> and args_ptrs: Vec<*mut c_char>. CString is owning just like Vec<u8> is.
There was a problem hiding this comment.
But how do you get from CString to *mut c_char?
There was a problem hiding this comment.
Oh sorry, .into_raw() is wrong since it takes self. But you can use .as_ptr() from CStr that takes &self.
There was a problem hiding this comment.
That gives you an immutable pointer, and casting that to mutable could introduce UB if the user code writes to it.
2 participants
mainleaks memory ofargs#649.I opted to store the final strings as
Vec<u8>rather than asCString, because the latter does not have a way to get a mutable pointer/reference without leaking it. This is because of the nul termination invariant thatCStringmust uphold.