Upgrade CString to not allocate on empty. #40547

clarfonthey · 2017-03-15T14:36:36Z

This is done by letting CString contain an empty slice, while also returning a static, NUL-terminated slice when necessary.

This shouldn't be too much of a maintenance burden, but I'll put this PR up and let the libs team decide if they want to keep this.

rust-highfive · 2017-03-15T14:36:48Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

nagisa · 2017-03-15T15:41:47Z

src/libstd/ffi/c_str.rs

-        let slice = slice::from_raw_parts(ptr, len as usize);
-        CString { inner: mem::transmute(slice) }
+        if len == 1 {
+            debug_assert_eq!(ptr, NUL_TERMINATED_EMPTY.as_ptr() as *mut c_char);


This assertion makes no sense. It is requiring all ptrs pointing to null byte be NUL_TERMINATED_EMPTY, which is a breaking change and also weird.

(It precludes code like one below from working)

let cstring = CString::from_raw(b"\0"); ... mem::forget(cstring);

nagisa · 2017-03-15T15:46:22Z

src/libstd/ffi/c_str.rs

@@ -274,7 +287,12 @@ impl CString {
    /// Failure to call `from_raw` will lead to a memory leak.
    #[stable(feature = "cstr_memory", since = "1.4.0")]
    pub fn into_raw(self) -> *mut c_char {
-        Box::into_raw(self.into_inner()) as *mut c_char
+        if let Some(bytes) = self.into_inner() {
+            if bytes.len() > 1 {


I do not see a reason for this conditional. Return the pointer always. Alternatively assert that if bytes.len() == 1, the pointer is the same as of NUL_TERMINATED_EMPTY slice.

That is already an assertion; I accidentally added that condition. Will remove.

oli-obk · 2017-03-15T18:55:58Z

Was CString NonZero before this change? It won't be after the change.

Transmute::<Option, Something>(a) will stop compiling, too, if it compiled before...

mzji · 2017-03-15T19:15:04Z

src/libstd/ffi/c_str.rs

 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct CString {
    // Invariant 1: the slice ends with a zero byte and has a length of at least one.
    // Invariant 2: the slice contains only one zero byte.
    // Improper usage of unsafe function can break Invariant 2, but not Invariant 1.
-    inner: Box<[u8]>,
+    inner: Option<Box<[u8]>,


Looks like lacks a > here?

clarfonthey · 2017-03-15T20:08:57Z

@oli-obk It was because it was just a wrapper for a Box, so, that is something to consider. In the future I can imagine that we'd like to back CString with a Vec instead of a Box to bring its API up to the same level as OsString and String, and if that happened, we'd have a similar problem.

I've also fixed the errors; initially I had also coded this change to replace Box with Vec, then changed it back last minute, which is why there were so many mistakes. I figured that I'd offer this change first because it doesn't require future plans for new APIs.

arthurprs · 2017-03-15T20:38:52Z

src/libstd/ffi/c_str.rs

@@ -260,8 +267,13 @@ impl CString {
    #[stable(feature = "cstr_memory", since = "1.4.0")]
    pub unsafe fn from_raw(ptr: *mut c_char) -> CString {
        let len = libc::strlen(ptr) + 1; // Including the NUL byte


Should the strlen be moved the into the else branch?

yes it should!

mzji · 2017-03-15T21:35:59Z

How about change CString to something like

enum CString {
    Empty([u8; 1]),
    Some(Box<[u8]>),
}

and initialize it to Empty([0]) for empty strings? This approach looks like more cache-friendly.

abonander · 2017-03-15T23:00:29Z

@mzji That won't work because CString needs to support being leaked for passing "ownership" over FFI, so giving out a pointer to the stack isn't what we want here.

abonander · 2017-03-15T23:07:02Z

To preserve null-pointer optimization, you can keep the original Box<[u8]>/Vec<u8> structure and then just don't push a zero-byte if the provided data is empty. Then in into_ptr(), CStr::to_ptr() and *_with_nul() methods, return a pointer to the static [0] if the data is empty.

mzji · 2017-03-15T23:22:18Z

@abonander We cannot pass the ownership of both stack memory and static memory over the FFI boundary, right?

abonander · 2017-03-15T23:32:44Z

@mzji Static memory is fine because it's guaranteed to be around for the life of the program. There is some issue if the consumer of the string means to mutate it (because the static will be stored in a non-writable region, and also potentially shared with other instances), but that's not commonly done, to my knowledge; string manipulations generally involve copying to a new allocation. It's also meaningless to mutate a zero-length C-string because all you could mutate is the nul-byte, in which case your handling of C-strings is erroneous to begin with, and a segfault from attempting to write non-writable memory would probably be the nicest way to discover that bug.

Leaking heap memory is also fine because it won't ever be reclaimed unless you get the pointer back and free it yourself. That's the point of CString::into_ptr().

However, stack memory is an issue because if you're wanting to pass a C-string to some library which will then hold onto it, having the backing memory on the stack is an issue because when your function returns and another one is called, you'll have some other code using the exact same memory region as your C-string.

mzji · 2017-03-15T23:36:53Z

@abonander Ok the 'C code will hold the pointer' part makes sense. Anyway that's shouldn't be called 'passing ownership', since the buffer will be freed by rust code, not C code. I misunderstood this.

abonander · 2017-03-15T23:41:09Z

That's why I put "ownership" in quotes. Even if the concept of ownership isn't built into the language, the C code (or any other language that's ABI-compatible with C) is still taking responsibility for the pointer and freeing it prematurely could cause Bad Things™ to happen.

mzji · 2017-03-15T23:44:21Z

What I thought is actually that the C code just takes a borrow (in Rust meaning) of the string. However, yeah, some C code just does some cute bad things... So put empty string in static memory could almost immidiately figure out some C code just works incorrectly.

abonander · 2017-03-15T23:45:32Z

If the FFI code doesn't assume the pointer will remain valid after the call returns, then passing a temporary pointer is fine; you would use CStr::as_ptr() there.

mzji · 2017-03-15T23:48:57Z

That's why I put "ownership" in quotes. Even if the concept of ownership isn't built into the language, the C code is still taking responsibility for the pointer and freeing it prematurely would cause Bad Things™ to happen.

Also one thing we should remember, is that the memory should be freed by the allocator which allocates it. That means, even for pointers generated by CString::into_ptr(), C code shouldn't free them, except the C code will call the allocator provided by Rust code. Noted.

sfackler · 2017-03-16T00:50:19Z

How often are empty strings passed through FFI? Is there a specific use case this is targeting?

dgel · 2017-03-17T10:17:10Z

I'm not sure if this scenario is a concern for rust libraries, but potentially you could load a dynamic library at runtime using dlopen, call a function returning an empty string, unload it and then try to access the string.
Disregarding deallocation, it might be nice if this doesn't crash on accessing the returned string

nagisa · 2017-04-07T08:20:55Z

Static memory is fine because it's guaranteed to be around for the life of the program.

This is not true. The dynamic libraries and their static memory may be loaded and unloaded at will.

All in all, I find it ever so harder to justify making this change.

alexcrichton · 2017-04-12T22:07:38Z

We discussed this PR during libs triage the other day, and we were in general hesitant to accept this due to the difference from how types like String and Vec operate.

@clarcharr can you clarify use cases and perhaps provide some real-world benchmarks to motivate this PR?

shepmaster · 2017-04-21T14:24:30Z

@clarcharr thanks for the contribution! We're looking for some clarification; would you mind responding to the previous comment?

clarfonthey · 2017-04-22T04:33:55Z

@shepmaster @alexcrichton sorry for not responding! I kind of forgot about this PR and I'm not 100% sure if I have a compelling enough argument for it.

Right now, my main thoughts were that it would support a future implementation of CString which allows Vec-type operations like String. It makes sense to not allocate by default because allowing manipulations of CStrings would make it very natural to include an empty CString in a constructor for a type, then later modify that string in future method calls. I can't think of a lot of cases where I'd necessarily do that, but then again, I consider CString to be one of the less-used types in stdlib.

Uses of that would basically be anywhere where some Rust code needs to build strings for C code. Which, again, I'm not sure how common that is.

I'd be willing to come up with some benchmarks if this kind of thing is desired, although right now I am leaning toward closing this and starting more of a discussion elsewhere on how desired Vec-like manipulation of CString is.

clarfonthey · 2017-04-22T04:38:06Z

Also for context, I've currently been looking up inconsistencies for rust-lang/rfcs#1876 that can be fixed now without an RFC, so that there's less that'd be needed to talk about for a future RFC. I sort of included this in that list as a "maybe we want this" because I figured that it'd be easier to come up with an implementation than to speculate about how it'd be done.

alexcrichton · 2017-04-24T14:48:59Z

Hm ok. I figured that we wouldn't end up expending that much effort enhancing CString, but rather if you're working with C strings and building them you'd quickly move to the Rust world of slices and Vec. Is there a concrete benefit from enhancing CString with various operations like on Vec and whatnot? It seems like we could just expose into_vec on CString (I don't think we currently have that?) in which case we wouldn't need to enhance the C string API?

clarfonthey · 2017-04-24T17:17:35Z

We currently do have that method, but the main downside I see is that you either a) have to go unsafe and assert yourself that there are no internal NULs or b) pay an extra penalty checking yourself.

I also figure that this is similar to what you get from String, where you either have to go unsafe or pay an extra penalty if you convert to Vec.

And considering how they'd all just be shallow façades over Vec methods anyway I don't think it'd be too unreasonable to add them.

alexcrichton · 2017-04-26T14:37:44Z

Ok well in any case the original thoughts of the libs team were that lacking motivation/benchmarks we'd be inclined to close this PR. Would you like to gather that though before closing?

clarfonthey · 2017-04-26T22:42:06Z

Personally I think it's best to close this now and wait until more discussion/an RFC happens before continuing. I can reopen later if there's a desire to merge this later.

rust-highfive assigned brson Mar 15, 2017

nagisa reviewed Mar 15, 2017

View reviewed changes

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 15, 2017

mzji reviewed Mar 15, 2017

View reviewed changes

clarfonthey force-pushed the c_string branch from 52a4ba1 to ada1a4e Compare March 15, 2017 20:04

arthurprs reviewed Mar 15, 2017

View reviewed changes

clarfonthey force-pushed the c_string branch 2 times, most recently from 766957d to 4417e8b Compare March 15, 2017 23:53

clarfonthey force-pushed the c_string branch 2 times, most recently from 8c89b4d to 063cfe8 Compare April 6, 2017 18:30

Upgrade CString to not allocate on empty.

9edf46a

clarfonthey force-pushed the c_string branch from 063cfe8 to 9edf46a Compare April 6, 2017 18:54

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

clarfonthey closed this Apr 26, 2017

clarfonthey deleted the c_string branch April 15, 2018 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade CString to not allocate on empty. #40547

Upgrade CString to not allocate on empty. #40547

clarfonthey commented Mar 15, 2017 •

edited

Loading

rust-highfive commented Mar 15, 2017

nagisa Mar 15, 2017

nagisa Mar 15, 2017

clarfonthey Mar 15, 2017

oli-obk commented Mar 15, 2017 •

edited

Loading

mzji Mar 15, 2017

clarfonthey commented Mar 15, 2017

arthurprs Mar 15, 2017

clarfonthey Mar 15, 2017

mzji commented Mar 15, 2017

abonander commented Mar 15, 2017

abonander commented Mar 15, 2017 •

edited

Loading

mzji commented Mar 15, 2017

abonander commented Mar 15, 2017 •

edited

Loading

mzji commented Mar 15, 2017

abonander commented Mar 15, 2017 •

edited

Loading

mzji commented Mar 15, 2017 •

edited

Loading

abonander commented Mar 15, 2017

mzji commented Mar 15, 2017 •

edited

Loading

sfackler commented Mar 16, 2017

dgel commented Mar 17, 2017

nagisa commented Apr 7, 2017

alexcrichton commented Apr 12, 2017

shepmaster commented Apr 21, 2017

clarfonthey commented Apr 22, 2017

clarfonthey commented Apr 22, 2017

alexcrichton commented Apr 24, 2017

clarfonthey commented Apr 24, 2017

alexcrichton commented Apr 26, 2017

clarfonthey commented Apr 26, 2017

Upgrade CString to not allocate on empty. #40547

Upgrade CString to not allocate on empty. #40547

Conversation

clarfonthey commented Mar 15, 2017 • edited Loading

rust-highfive commented Mar 15, 2017

nagisa Mar 15, 2017

Choose a reason for hiding this comment

nagisa Mar 15, 2017

Choose a reason for hiding this comment

clarfonthey Mar 15, 2017

Choose a reason for hiding this comment

oli-obk commented Mar 15, 2017 • edited Loading

mzji Mar 15, 2017

Choose a reason for hiding this comment

clarfonthey commented Mar 15, 2017

arthurprs Mar 15, 2017

Choose a reason for hiding this comment

clarfonthey Mar 15, 2017

Choose a reason for hiding this comment

mzji commented Mar 15, 2017

abonander commented Mar 15, 2017

abonander commented Mar 15, 2017 • edited Loading

mzji commented Mar 15, 2017

abonander commented Mar 15, 2017 • edited Loading

mzji commented Mar 15, 2017

abonander commented Mar 15, 2017 • edited Loading

mzji commented Mar 15, 2017 • edited Loading

abonander commented Mar 15, 2017

mzji commented Mar 15, 2017 • edited Loading

sfackler commented Mar 16, 2017

dgel commented Mar 17, 2017

nagisa commented Apr 7, 2017

alexcrichton commented Apr 12, 2017

shepmaster commented Apr 21, 2017

clarfonthey commented Apr 22, 2017

clarfonthey commented Apr 22, 2017

alexcrichton commented Apr 24, 2017

clarfonthey commented Apr 24, 2017

alexcrichton commented Apr 26, 2017

clarfonthey commented Apr 26, 2017

clarfonthey commented Mar 15, 2017 •

edited

Loading

oli-obk commented Mar 15, 2017 •

edited

Loading

abonander commented Mar 15, 2017 •

edited

Loading

abonander commented Mar 15, 2017 •

edited

Loading

abonander commented Mar 15, 2017 •

edited

Loading

mzji commented Mar 15, 2017 •

edited

Loading

mzji commented Mar 15, 2017 •

edited

Loading