One of the programs I wrote had a serious runtime bug, and it was hidden and malfunctioning. The problem is that in a fully SAFE mode part. The lifetime of the variable 'txt' is not bound to the variable 'ptr' and thus the programmer, when writing his code in SAFE mode, can commit a serious memory error undetected. I feel that the Rust principle is violated here, but I'm not sure if I'm right.
However, I feel it is important to report this ambiguity because more programmers may make this serious error.
Update: please have a look at this, because the original problem NOT in the print_debug() func. If I don't use this debug function, the memory problem is the same. Here is a code and generated assembly without unsafe environment: Lifetime bug in safe part (String - ptr)? - #7 by Zsolt
This is why dereferencing a pointer is considered unsafe; as long as you don't dereference the dangling pointer you've created, you've not committed a memory error by Rust's rules; you have a pointer that's pointing at nothing (since the String it points at has been freed), and thus must not dereference it, but you can continue to pass it around until then.
Rust provides references, which prohibit this error, but if you're choosing to use raw pointers, you're implicitly accepting that you're responsible for making sure the pointer doesn't dangle.
print_debug is what we call "unsound". In other words: There is a way to call it using safe code that causes it to perform UB. You either have to mark print_debug as unsafe and document that the argument must be a valid pointer or you have to change the argument type to something that guaranteed that the pointer is valid and readable.
The Safety comment is missing, so we put it on there:
// Safety: We must have ensured ptr is valid and not dangling, not mutably aliased, etc,
// so it is dereferenceable (3)
let val = unsafe { *ptr }; // first character
Then we follow the review upwards:
// We ensure the ptr is non-null - that's one kind of invalid pointer
// removed from the equation (2)
if !ptr.is_null() {
// This is valid for all pointers, no problem here
println!("ptr: {ptr:?}");
// print_debug is a safe function, so it accepts any raw pointer
// there is no safety contract here. (1)
fn print_debug(ptr: *const u8) {
We find a contradiction: (1) and (2) combined do not ensure the necessary conditions for (3) to be valid. We have checked for null pointers, but we didn't (and there is about no way to do so with a conditional!) check if the pointer was valid to dereference.
No, the problem IS the print_debug() function, because its signature is essentially sayin:
no matter what pointer is given as input, even invalid pointers, this function is safe to call and won't produce UB
I hope you can see this is clearly false, because it does produce UB if a dangling or invalid pointer is passed in, as you did in the other safe function.
Ultimately you should not assume raw pointers to be valid, and if you use unsafe to read from them without having the guarantee that they are valid then that unsafe code is at fault.
let txt;
let ptr = if !s.is_empty() {
txt = s.to_string() + "BC";
txt.as_ptr()
}
// ptr pointed an alive "txt", so the contents of RAM is valid.
The content of RAM which pointed of ptr good (watch it with a low level debugger).
let ptr = if !s.is_empty() {
let txt = s.to_string() + "BC";
txt.as_ptr()
}
// ptr pointed a freed RAM in this SAFE environment.
The content of RAM which pointed a ptr is bad (watch it with a low level debugger), because the ptr variable is alive but the 'txt' is freed. This should only happen in unsafe environments, but this part of code is in SAFE environment, so I don't assumed, that this part maybe a free memory problem.
I wrote another code to illustrate the lifetime problem. If I return with String too, the pointed content is good, but if the String is only a local variable, the content of RAM is bad, because of freed.
This code contains ONLY SAFE rows, but here will be the values bad.
In my opinion, such a bug should only occur in an unsafe environment. Otherwise, I cannot trust the whole code base, even though by default I only focused on the unsafe parts when searching for memory errors.
By using a raw pointer, instead of a reference, you've told Rust that you will come up with your own scheme for ensuring that the contents of memory are not bad at the time print_debug is called. That's one of the differences between Rust pointers and Rust references.
You've then not done this, and used unsafe to tell Rust that you've done something it can't check and thus been able to guarantee that the memory pointed to by the pointer is good at the time print_debug dereferences it.
This is intended behaviour; when the String goes out of scope, it is dropped, and as part of dropping, it frees all the memory it owns. To avoid the memory being freed too early, you need to prevent the String from being dropped "too early".
One route, and the one normally recommended in Rust, is to use references; then, the borrow checker confirms, at compile time, that the String is dropped after all references to that String are dropped. If the borrow checker can't prove this, you get a compile error telling you that there's a problem with your reference.
Another route is to pass ownership around - either by returning the String, or by using a construct like Rc<String> or Arc<String> to share ownership between multiple places. This is also checked for you, and will not compile if you're depending on the String being kept around for longer than it actually is.
You can also come up with more complicated schemes; Rust provides pointers for the case where you've come up with a complicated scheme that it can't check. If you're using pointers, then Rust requires that you confirm, using the magic word unsafe, that you've come up with a complicated scheme to keep the thing you are pointing to from being dropped earlier than expected; nothing goes wrong in safe code, because it's fine for pointers to point at bad memory, it's just not OK for you to dereference pointers to bad memory (and that operation requires unsafe).
As a result, unsafe tends to "spread out"; at the point you use an unsafe superpower, you're promising Rust that the documented but unchecked requirements of that operation are met by the surrounding code in all cases. Many Rust projects require safety comments which document how you've met the unchecked requirements of an unsafe superpower; additionally, if you need your callers to meet some unchecked requirements, you mark your function as unsafe, and document the requirements that callers must meet in a safety comment.
In the case of your original code, the safe code does nothing wrong - there's no safe code that requires the String to keep the memory it owns alive, so it's fine to free it. However, print_debug has a requirement that the pointer you pass it points to valid memory; you can't check this inside print_debug, so print_debug needs to be unsafe, and to have a safety comment telling you that the pointer must point to valid memory.
And as_ptr does not promise to keep the memory it points to alive; it gives you a raw pointer to an existing block of memory, and it's on you to keep that memory in a good state somehow. Rust explicitly doesn't guarantee that it will do this with raw pointers, to keep them as flexible as possible for the case where you're doing something extremely clever that you simply cannot explain to the compiler in source code.
No, it's perfectly ok for this to happen with raw pointers, not matter if you're in a safe or unsafe environment. If you want something a pointer that is guaranteed to be valid you want a reference.
Also, this is ultimately the reason why the code produces UB, but your print_debug would be wrong with or without this piece of code.
But there is an error in print_debug, and if you look for unsafe code you will find it. If you correctly verify all unsafe code then you can actually trust the whole codebase to be safe, but you do need to consider code like print_debug to be wrong/buggy/unsound.
I'm sorry, even though there is a chain of events required to make this bug happen, by Rust's safety model the link in the chain that breaks here - and is at fault - is the print_debug function. It's an unsound function, and bjorn3 suggested how to fix it.
I think you are right, you have to start from the unsafe code when debugging a problem. So you have to start from the unsafe code block in print_debug and see what has been done to ensure its requirements are in place.
To avoid a fundamental mistake in the reasoning: I wouldn't call it an unsafe mode. The keyword does not modify the operational semantics of the code that's in its block. It only asserts that the programmer has taken care that the code's semantics are sound in combination, allowing the use of some operations where the compiler does not / can not take care of this step for them. This responsibility includes ensuring the fundamental property of sound code: that it remains free of UB in any composition with other sound code.
fn bad_func(s: &str) {
let mut capacity = 0;
let mut len = 0;
let ptr = if !s.is_empty() {
// `String` is allocated
let txt = s.to_string() + "BC";
// We get a pointer to the `String`, with no lifetime attached to it
let ptr = txt.as_ptr();
capacity = txt.capacity();
len = txt.len();
// We leak the memory for the `String`, so it doesn't get deallocated, leaving the pointer as valid
std::mem::forget(txt);
ptr
} else {
std::ptr::null()
};
// The pointed to memory is still valid because it wasn't deallocated
print_debug(ptr); // show the content
if capacity != 0 {
// We reconstruct the `String` and drop it to deallocate it, otherwise the memory is leaked for the rest of the application run time
drop(unsafe { String::from_raw_parts(ptr as _, len, capacity) });
}
}
This is conceptually equivalent to what the borrow checker would do with "lifetime extension" (if it allowed to take a reference to txt and automatically make it last as long as needed for the references to be valid), in the first version txt isn't deallocated until after print_debug is called, while your second version deallocates the String immediately after the if expression, because the compiler doesn't know there are any dangling pointers to the data, that's what references are for and the borrow checker will correctly complain about it:
error[E0597]: `txt` does not live long enough
--> src/main.rs:35:9
|
31 | let ptr = if !s.is_empty() {
| --- borrow later stored here
...
34 | let txt = s.to_string() + "BC";
| --- binding `txt` declared here
35 | txt.as_str()
| ^^^ borrowed value does not live long enough
36 | } else {
| - `txt` dropped here while still borrowed
As a note for the team, it would be great if we had some basic lifetime information attached to pointers for the purposes of linting against simple cases like the one presented here. It's come up before in conversation, and I'd still like it.