Consider returning &c_char from CString::as_ptr

[Ms2ger]

This should make it a lot harder to run into the bug mentioned in its documentation (let p = foo.to_c_str().as_ptr();).

[pcwalton]

+1

[metajack]

We hit this a ton in Servo in this latest rust upgrade, which caused many use after free bugs in otherwise safe code.

I think as_ptr() should be removed, and replaced with as_ref(), so the lifetime annotations stick and prevent this easy-to-make error.

[pcwalton]

cc @aturon

[pcwalton]

nominated for 1.0 P-backcompat-libs

[aturon]

The return type should be &[c_char] rather than &c_char, I'm assuming?

Just to clarify, the as_bytes and as_bytes_no_nul methods already provide this functionality.

I'd like to understand better how CString is being used in Servo. Within libstd, the predominant use is cases where you need to get a *const i8 to pass along to some C bindings. (When consuming C strings, we generally use from_c_str, which has recently been renamed to from_buf.)

I could imagine trying to do something more RAII-ish here, and return an object that derefs into *const i8, which would probably help catch some of these bugs.

Thoughts?

cc @alexcrichton

[metajack]

Here are some examples:

https://github.com/servo/servo/blob/rustup-20140716/src/components/script/dom/bindings/utils.rs#L241-L244

https://github.com/servo/servo/blob/rustup-20140716/src/components/script/dom/bindings/utils.rs#L241-L244

https://github.com/servo/rust-mozjs/blob/rustup-20140716/rust.rs#L137-L151

https://github.com/servo/rust-opengles/blob/rustup-20140716/gl2.rs#L381

[alexcrichton]

I believe the idea is that &c_char will implicitly coerce to *const c_char whereas &[c_char] will require another method call, sort of the difference between

foo(s.as_ref())
// vs
foo(s.as_bytes().as_ptr())

I'd be ok moving this over to &c_char, although I'd like to see the impact first to make sure it doesn't turn up too many surprises.

[pnkfelix]

P-backcompat-libs, but not 1.0: this is in part of the stdlib that is still tagged with experimental stability attribute.

[aturon]

@alexcrichton Should we repurpose this bug with the new CString API?

[alexcrichton]

This may be less relevant now that the API of CString has been pared back so much. It's much more clear that CString::from_slice(foo).as_ptr() is probably not going to outlive the expression thatn foo.to_c_str().as_ptr(), so I'd also just be fine closing this.

[Ms2ger]

From IRC today:
<noodle> Why are these two c-pointers the same? http://is.gd/2KlvtT

use std::ffi::CString;

fn main() {
    let a = "1";
    let b = "2";
    let a_ptr = CString::from_slice(a.as_bytes()).as_ptr();
    let b_ptr = CString::from_slice(b.as_bytes()).as_ptr();

    println!("{:?}", a_ptr);
    println!("{:?}", b_ptr);
}

[aturon]

@alexcrichton I'm not sure I agree, it seems like exactly the same pitfalls are there. I think we should consider adding a method like the one proposed in this issue.

[aturon]

Nominating for 1.0-beta (P-backcompat-libs, as it already is).

[pnkfelix]

1.0 beta, P-backcompat-libs, I-needs-decision,.

[aturon]

Decision: we want to make a change like this, but there are some other ramifications for using CString this way. In particular there are patterns around Option<CString> for representing nullable data and converting to a pointer that this would make very painful.

We want to introduce some general libraries to cover patterns like that before we make this change.

So all told, we're OK stabilzing as_ptr for 1.0, and then later deprecating in favor of as_ref when this other infrastructure is ready.

Closing as WONTFIX (for now).

[alexcrichton]

I have opened an RFC issue about this issue more broadly which may help alleviate some concerns here as well.