Lint against instantly-dangling pointers like String::with_capacity(MAX_PATH).as_mut_ptr()

[FlorentinoJink]

Issue Description

The following Rust code:

const MAX_PATH: usize = 260;
fn main() {
    let str1 = String::with_capacity(MAX_PATH).as_mut_ptr();
    let str2 = String::from("TotototototototototototototototototoT").as_mut_ptr();
    unsafe {
        std::ptr::copy_nonoverlapping(str2, str1, 30);
        println!("{:?}", String::from_raw_parts(str1,30,30));
    }
}

uses the String::with_capacity function to create a string with a capacity of 260 characters. It then uses the as_mut_ptr method to get a raw pointer to the string.

However, this approach can lead to undefined behavior because:

The String::with_capacity function only guarantees to allocate enough memory to store the specified number of characters. It does not guarantee that the allocated memory is valid.
The as_mut_ptr method returns a raw pointer to the internal data of the string. This pointer may point to uninitialized memory or memory that has been invalidated by other operations.
Therefore, using the String::with_capacity(MAX_PATH).as_mut_ptr() method to create a raw pointer can lead to the following problems
-Program crashes
-Data corruption
-Security vulnerabilities

Expected Behavior
The String::with_capacity function should not allow the creation of a raw pointer to uninitialized memory.

Similar to String::from("x").as_mut_ptr(), such raw pointers should not be compiled successfully.

[workingjubilee]

@FlorentinoJink The problem is not truly with_capacity or as_mut_ptr but your subsequent usage of copy_nonoverlapping and from_raw_parts on an invalid pointer. The program is perfectly sound until that point. Yes, invalid pointers are fairly useless, but they are often created for various reasons.

More unfortunately, a raw pointer does not have a lifetime. This is a fundamental reality of Rust. Code that requires eschewing lifetimes needs that. And you have chosen to eschew lifetimes, and then ignore the invariants of ptr::copy_nonoverlapping and String::from_raw_parts, which state that they require valid pointers as input. There are many functions on pointers that do not require valid pointers and in fact can be quite useful without them (though most of the uses I can immediately think of have now been superceded by offset_of!).

It is indeed an undesirable API but I do not see how we can realistically require it to not compile anymore, as this code, for instance, is perfectly reasonable, and also has no lifetime bound, for the same reason:

fn main() {
    let str1_len = String::from("does not live to the next line").len();
}

And you seem to claim this code does not compile? Or perhaps that was a "should not"?

fn main() {
    let str1 = String::from("x").as_mut_ptr();
}

Unfortunately, it does.

Ultimately, you have not fully explained how the safe functions in question are at fault, you have only specified how irresponsible use of unsafe code can cause this problem. It is undesirable that the current APIs are shaped as they are, and I would not have chosen them, to be sure. But I do not think we can simply break them, as we promise we do not alter the type signatures of stable interfaces.

[workingjubilee]

The String::with_capacity function only guarantees to allocate enough memory to store the specified number of characters. It does not guarantee that the allocated memory is valid.

To be clear, it sure as hell does allocate valid memory, the problem is that the string doesn't last past the end of that expression so you created a pointer to some valid memory, and then it becomes invalid just as quickly.

[saethlin]

We should have a lint against this pattern of creating an instantly-dangling pointer. I've seen this pattern mistakenly used in unit tests; it's a funny sort of canary for people who write unsafe code but didn't use ASan before publishing their work.

[FlorentinoJink]

Thank you for your patient explanation. I will be more careful when using raw pointers in the future.

[workingjubilee]

@saethlin Hmm. It would be worthwhile if possible but how would we do this without also linting on e.g. hand-implementations of offset_of! that exist in the wild? Do we only do it for standard library types, do we make the lint passes aware of deallocation...?

I suppose we could have the Rust wrapper of the system allocator contain debug assertions (e.g. this particular example winds up deallocating the same pointer twice for a double-free, the second time after the String::from_raw_parts call, which is recreated with an invalid layout: 30 is not a valid capacity to pass).

[saethlin]

What I'm suggesting is just a lint against the specific pattern of calling a constructor function then a function called as_ptr or as_mut_ptr, then a semicolon. What I'm imagining targeting here is just the case where someone creates a String or a Vec but then their IDE suggests that they apply as_ptr or as_mut_ptr as a conversion function, because the IDE is stupid and just playing Type Tetris.

[saethlin]

I do not think that the scenario as described in the initial issue description is a reasonable thing to detect and warn about in the frontend. We have Miri and ASan for that.

[jendrikw]

We already lint temporary-cstring-as-ptr, so linting similar situations with String and Vec don't seem far-fetched.

[GrigorenkoPV]

What I'm suggesting is just a lint against the specific pattern of calling a constructor function then a function called as_ptr or as_mut_ptr, then a semicolon.

We already lint temporary-cstring-as-ptr, so linting similar situations with String and Vec don't seem far-fetched.

@rustbot claim

One question though, there is no simple way for a lint to check that a value is a temporary? Something like is_rvalue in C++ parlance. Or is there?

The current implementation of temporary-cstring-as-ptr seems to only match the CString::new(...).unwrap() or CString::new(...).expect(...). And trying to extend this to other containers by manually matching on all possible constructors does not sound that appealing. So I was wondering if there is a better way.

[workingjubilee]

...do we have a concept of borrow-checker lints...? because it seems it would be really easy to tell using MIR "oh, this is a pointer to a dead thing."

[saethlin]

That doesn't help for String::new().as_ptr(), the borrow checker has no idea that the pointer and String aren't connected, which is the point of that function.