Remove str trailing nulls #8296

erickt · 2013-08-05T01:39:46Z

This PR fixes #7235 and #3371, which removes trailing nulls from str types. Instead, it replaces the creation of c strings with a new type, std::c_str::CString, which wraps a malloced byte array, and respects:

No interior nulls
Ends with a trailing null

… str

When strings lose their trailing null, this pattern will become dangerous: let foo = "bar"; let foo_ptr: *u8 = &foo[0]; Instead we should use c_strs to handle this correctly.

…null

thestinger · 2013-08-05T02:39:43Z

I agree 100% with removing the legacy trailing \0 from our strings for the reasons given in my comments on the issue report.

It doesn't have to happen as part of this pull request, but we should really switch Path to using [u8] internally. At the moment it's not possible to handle files without Unicode paths, despite many filesystems allowing it. The same thing applies to environment variables, as they are not necessarily UTF-8.

I think we'll find most places using str for interaction with C were doing so incorrectly, especially since it's impossible for a function using \0-termination to support all of UTF-8. File paths are one of the few things really guaranteed to never contain \0.

I'll hold off on an r+ because this deserves more attention than most pull requests.

bluss · 2013-08-05T06:34:54Z

src/libstd/c_str.rs

+    /// # Failure
+    ///
+    /// Fails if the CString is null.
+    pub fn as_bytes(&self) -> &'self [u8] {


Shouldn't this be like this?

fn as_bytes<'r>(&'r self) -> &'r [u8]

brson · 2013-08-05T18:51:59Z

Really awesome effort.

With the CString type, when it doesn't own it's buffer is it true that any operation on it is effectively unsafe? So CString has a sort of safe/unsafe modality? If so that's kind of frightening.

erickt · 2013-08-05T19:58:38Z

@brson: Maybe I didn't document my intentions of CString owning or not owning the returned pointer well enough. One of the more obnoxious things to deal with in C is who owns the pointer. strcpy obviously returns a pointer that should be freed, but getenv returns a pointer that should not be freed. I wanted to allow library writers embed this information into CString.

That said, this api may be a little half baked at the moment and I'd be okay with removing this feature for now. Perhaps instead I should follow extra::c_vec and instead allow a user to pass in a destructor function to allow CString to work with other allocators.

brson · 2013-08-05T22:44:29Z

@erickt this is great progress, so even if the design needs to continue to evolve I'm pretty happy merging it. I think we probably need to bring it up in the meeting tomorrow though since this is a major language change.

sfackler · 2013-08-06T16:08:02Z

This will probably need to be rebased on top of #8289 and str::raw::push_byte tweaked or the test that that commit added will fail.

brson · 2013-08-06T16:18:03Z

@erickt can you merge again?

…tr-trailing-nulls

…trailing-nulls

graydon · 2013-08-07T23:52:27Z

Merging because this is a big annoying bitrotty change and feels like the main API surface of it won't change much in the future.

I too am a little concerned about ownership being controlled by a flag, and by the dtor calling free. I think this difference may be significant enough to represent at the type level? I'm unsure. I can think of a number of other ways of representing it, few of which seem like they are particularly safe / footgun-proof. I'd love to be able to leverage lifetimes for this, such that (say) a CStringOwner can produce a CStringRef<'r> that points to the former and couples their lifetimes together. Or something? This is basically akin to the slice problem we had in normal strings, but for strings-that-C-will-like. I'm not the best at rust API design, sadly. But it seems to me the shorter the duration raw pointers emerge from the API surface, the better.

Also somewhat concerned about str::to_c_str: it calls as_bytes().to_c_str() which doesn't wind up verifying that the string in question lacks internal nulls, or is valid in the target locale-encoding, both of which are motivating concerns for this work.

Also ergonomically a bit concerned with to_c_str().with_ref since it's a long and chatty invocation, and hits a path that definitely allocates an owning buffer even though it's clearly a temporary use. Could we add a to_c_str_ref() combination function that allocates into a temporary buffer (and say, uses a small stack-local array for strings less than 128 bytes? that's likely most of them..)

…trailing-nulls

This PR fixes #7235 and #3371, which removes trailing nulls from `str` types. Instead, it replaces the creation of c strings with a new type, `std::c_str::CString`, which wraps a malloced byte array, and respects: * No interior nulls * Ends with a trailing null

bluss · 2013-08-10T05:01:21Z

mini-regression, the recent function signature change of std::str::raw::slice_bytes is reverted. Can be fixed later.

erickt · 2013-08-10T15:38:27Z

@blake2-ppc: thanks for catching that. I folded a fix for it into #8430 to cut down on the bors queue.

Erik-S · 2013-08-11T18:48:29Z

src/libstd/c_str.rs

+    fn drop(&self) {
+        if self.owns_buffer_ {
+            unsafe {
+                libc::free(self.buf as *libc::c_void)


On Windows there is no guarantee that "free" is the correct function to use to deallocate an OS-supplied buffer. Typically each API or set of APIs will have its own free-equivalent (but you have to call the correct one, or everything may break).

Raymond Chen (at the Old New Thing blog) has lots more details: http://blogs.msdn.com/b/oldnewthing/archive/2006/09/15/755966.aspx

@Erik-S I'm afraid that nobody will see a comment on a closed pull request, can you file an issue for this?

Good point. I opened issue #8465

@graydon

One of the downsides with `c_str` is that it always forces an allocation, and so this could add unnecessary overhead to various calls. This PR implements one of the suggestions @graydon made in #8296 for `vec.with_c_str` in that for a short string can use a small stack array instead of a malloced array for our temporary c string. This ends up being twice as fast for small strings. There are two things to consider before landing this commit though. First off, I arbitrarily picked the stack array as 32 bytes, and I'm not sure if this a reasonable amount or not. Second, there is a risk that someone can keep a reference to the interior stack pointer, which could cause mayhem if someone were to dereference the pointer. Since we also can easily grab and return interior pointers to vecs though, I don't think this is that much of an issue.

alexisvl · 2017-04-13T15:21:10Z

"Oh, you can just malloc a new string with the null termination", said the developers who have never seen a microcontroller. "Screw anybody using Rust in an embedded environment and needing to interoperate with C."

Seriously, sometimes RAM is literally that tight. This does nothing but go out of your way to hassle people using Rust in low memory environments. Thanks, now I have to go write a special allocator to copy small strings and terminate them without fragmenting my handful of kB.

new lint: `missing_asserts_for_indexing` Fixes rust-lang#8296 This lint looks for repeated slice indexing and suggests adding an `assert!` beforehand that helps LLVM elide bounds checks. The lint documentation has an example. I'm not really sure what category this should be in. It seems like a nice lint for the `perf` category but I suspect this has a pretty high FP rate, so it might have to be a pedantic lint or something. I'm also not sure about the name. If someone knows a better name for this lint, I'd be fine with changing it. changelog: new lint [`missing_asserts_for_indexing`]

erickt added 15 commits August 4, 2013 13:32

std: minor cleanup

6011f83

std: add str.to_c_str()

08b6cb4

std: rewrite run::with_{argv,envp,dirp} to copy C strings

fd293df

std: remove str::NullTerminatedStr

dca9ff9

std: cleanup os and str tests

cd94e91

std: add test for str::as_c_str

d511085

std: remove str::from_bytes_with_null

bb5bf7c

extra: make sure time::match_digits does not read past the end of the…

0512475

… str

std: replace str::as_c_str with std::c_str

3102b17

std and rustc: explicitly pass c strings to c functions

bd908d4

When strings lose their trailing null, this pattern will become dangerous: let foo = "bar"; let foo_ptr: *u8 = &foo[0]; Instead we should use c_strs to handle this correctly.

std: merge str::raw::from_buf and str::raw::from_c_str

3629f70

std: remove use of cast module from os.

17e0089

Remove trailing null from strings

5865a75

Merge remote-tracking branch 'remotes/origin/master' into str-remove-…

3c94b50

…null

std: Update the c_str docs, and support CString not owning the pointer

986ba9c

bluss reviewed Aug 5, 2013
View reviewed changes

erickt mentioned this pull request Aug 5, 2013

str::from_bytes #2268

Closed

erickt added 2 commits August 5, 2013 19:54

std: fix a typo where .to_c_str wasn't being called on android

c8e4540

std: c_str should use regions on methods

fb9b279

lilyball mentioned this pull request Aug 6, 2013

run: Process::new() should take &[Str] instead of &[~str] for args #8203

Closed

erickt added 3 commits August 6, 2013 16:18

Merge commit 'd89ff7eef969aee6b493bc846b64d68358fafbcd' into remove-s…

8567611

…tr-trailing-nulls

std: update str.push_byte to work without str trailing nulls

5e7b666

Merge remote-tracking branch 'remotes/origin/master' into remove-str-…

5eaa4d1

…trailing-nulls

Fix a stack to use the new .to_c_str() api

e7f0882

std: import HANDLE for os::list_dir for windows

7a5ee37

std: more fixes for os.rs for windows

03cc757

Merge remote-tracking branch 'remotes/origin/master' into remove-str-…

56730c0

…trailing-nulls

std: more windows fixes to os.rs and run.rs

c14e14e

erickt added 2 commits August 9, 2013 14:06

rustpkg: another fix for windows

cab6d46

Merge remote-tracking branch 'remotes/origin/master' into remove-str-…

ee59aac

…trailing-nulls

bors closed this Aug 10, 2013

huonw mentioned this pull request Aug 10, 2013

Make str::as_buf less error prone. #3371

Closed

Erik-S reviewed Aug 11, 2013
View reviewed changes

Erik-S mentioned this pull request Aug 12, 2013

Let user specify function to use for freeing foreign buffers #8465

Closed

erickt mentioned this pull request Sep 20, 2013

Micro-optimize vec.with_c_str for short vectors #9352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove str trailing nulls #8296

Remove str trailing nulls #8296

erickt commented Aug 5, 2013

thestinger commented Aug 5, 2013

bluss Aug 5, 2013

brson commented Aug 5, 2013

erickt commented Aug 5, 2013

brson commented Aug 5, 2013

sfackler commented Aug 6, 2013

brson commented Aug 6, 2013

graydon commented Aug 7, 2013

bluss commented Aug 10, 2013

erickt commented Aug 10, 2013

Erik-S Aug 11, 2013

bstrie Aug 12, 2013

Erik-S Aug 12, 2013

alexisvl commented Apr 13, 2017

Remove str trailing nulls #8296

Remove str trailing nulls #8296

Conversation

erickt commented Aug 5, 2013

thestinger commented Aug 5, 2013

bluss Aug 5, 2013

Choose a reason for hiding this comment

brson commented Aug 5, 2013

erickt commented Aug 5, 2013

brson commented Aug 5, 2013

sfackler commented Aug 6, 2013

brson commented Aug 6, 2013

graydon commented Aug 7, 2013

bluss commented Aug 10, 2013

erickt commented Aug 10, 2013

Erik-S Aug 11, 2013

Choose a reason for hiding this comment

bstrie Aug 12, 2013

Choose a reason for hiding this comment

Erik-S Aug 12, 2013

Choose a reason for hiding this comment

alexisvl commented Apr 13, 2017