Skip to content

Conversation

@ricked-twice
Copy link

During an internship studying Rust compilation phase, I encounter this problem with std::ptr::read function.
As I read this guide, I thought I could contribute to it.
So this is a presentation of my contribution to this guide (english and french).

I extended the chapter 4 of the book, and added a rule about the usage of the std::ptr::read function.
As explained, a usage with a type implementing the Drop trait could lead to double-drop and possibly a double-free if the value is stored on the heap and freed during drop execution.

This kind of contribution should have followed an issue, but I have been told that it would be easier for maintainers to directly go for a pull request. So here it is! 😄

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea to insist on this point 💯

We could maybe even mention that in the rare case where ptr::read is needed, then:

  • either use a pre-emptive ManuallyDrop of the original value;

  • or ptr::write-overwrite the original value before it is used. This last case should be handled with great caution, though, given how minimal exception safety / unwind safety may be broken otherwise (e.g., a panic before the write will still lead to a double drop; thus the write should happen in a "finally" like block, such as one from ::scopeguard (or using catch_unwind)); code snippets of https://docs.rs/replace_with being very educational in that regard.


I still think the suggested documentation should be a bit more harsh as to how bad misusing this function is, given how many things can go wrong, and how wrong that can be (UAF / double free can cause remote code execution), hence my request for changes 😉

  • (Note that I am just a random user, I am not part of the maintainers of the repo).

@ricked-twice
Copy link
Author

ricked-twice commented May 20, 2022

Okay, so finally I should have took last reviews into account (with a lot of commits, sorry about that...).
It only took me a year or so 👍

Comment on lines +258 to +262
fn main(){
let obj: MyStruct = MyStruct(100);
let ptr: *const MyStruct = &test as *const MyStruct;
println!("{:?} @ {:p}", unsafe { std::ptr::read(ptr) }, ptr);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Donner le résultat obtenu après exécution du programme

Comment on lines 227 to 313
## Déplacement de valeurs

Rust propose trois différents modes de déplacement de valeur:

- Soit par *déplacement*, qui est le comportement par défaut.
- Ou par *déplacement* plus un *drop* si `mem::needs_drop::<T>()` renvoie `true`.
- Ou par *copie*, si son type implémente le trait `Copy`

Cependant, des problèmes peuvent être constater lors de l'utilisation de la fonction `std::ptr::read`.
Selon la [documentation](https://doc.rust-lang.org/std/ptr/fn.read.html), cette fonction:
> Lis la valeur pointée par src sans la déplacer. Ce qui laisse la mémoire pointée intact.
Cette fonction est donc responsable d'effectuer une copie de la valeur pointée, indépendamment du mode de déplacement du type en question.
Ce comportement peut être dangereux car il peut mener à des *double-free* et/ou des *double-drop*.

Pour illustrer ce comportement, considérons le code suivant :

```rust
# use std::ops::Drop;
#
#[derive(Debug)]
struct MyStruct(u8);

impl Drop for MyStruct {
fn drop(&mut self) {
# println!("---Dropping an object---\nBefore zeroing: {} @ {:p}", self.0, &self.0 as *const u8);
self.0 = 0;
# println!("After zeroing: {} @ {:p}", self.0, &self.0 as *const u8);
}
}

fn main(){
let obj: MyStruct = MyStruct(100);
let ptr: *const MyStruct = &test as *const MyStruct;
println!("{:?} @ {:p}", unsafe { std::ptr::read(ptr) }, ptr);
}
```

On peut observer qu'un deuxième objet a implicitement été créé lors de l'appel à `std::ptr::read`, i.e. une copie d'un objet *non copiable* est effectuée.

Cela peut poser différents problèmes :

- Dans certains _rares_ cas (pas de _drop glue_, ni de guarantie de non-aliasing, ni d'invariants de sûreté reliés au type; i.e. quand le type aurait dû être `Copy`), cela peut être sans danger.

- Dans la plupart des cas, il pourrait y avoir des invariants de sûreté ou de validité (tel que l'aliasing de pointeur) ce qui rend une instance `unsafe` à utilisé, voire même UB, au moment où l'autre instance est utilisée.

- Exemple:

```rust,no_run
# use ::core::mem;
#
let box_1: Box<i32> = Box::new(42);
let at_box_1: *const Box<i32> = &box_1;
let box_2: Box<i32> = unsafe { at_box_1.read() };
mem::forget(box_1); // `Box` non-aliasing guarantees invalidates the pointer in `box_2`
drop(box_2); // UB: "usage" of invalidated pointer.
```
Cf. [Stacked Borrows](https://plv.mpi-sws.org/rustbelt/stacked-borrows/) pour plus d'informations.
- Si le type doit être *drop* (i.e. `mem::needs_drop::<T>()` renvoie vraie), alors quand une des deux instances est *drop*, toute utilisation de l'autre peu causer un *use-after-free*, l'exemple le plus notable étant que la seconde instance sera également *drop*, ce qui causera un *double-free*.
**Ceci est un Comportement Non Défini (*Undefined Behavior*), et est responsable de vulnérabilités majeures**.
Le code suivant illustre ce problème :
```rust
# use std::boxed::Box;
# use std::ops::Drop;
#
#[derive(Debug)]
struct MyStructBoxed(Box<u8>);
impl Drop for MyStructBoxed {
fn drop(&mut self) {
# println!("---Dropping an object---\nBefore zeroing: {} @ {:p}", self.0, self.0);
let value: &mut u8 = self.0.as_mut();
*value = 0;
# println!("After zeroing: {} @ {:p}", self.0, self.0);
}
}
fn main(){
let test: MyStructBoxed = MyStructBoxed(Box::new(100));
let ptr: *const MyStructBoxed = &test as *const MyStructBoxed;
println!("{:?} @ {:p}", unsafe { std::ptr::read(ptr) }, unsafe { &*ptr }.0 );
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je pense que cette partie n'est pas nécessaire à la justification de la règle. En effet, un pointeur raw n'est, dans la pratique, pas utilisable sans unsafe.

Copy link
Contributor

@hg-anssi hg-anssi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. Even if the rule may be OK, the justification could be simplified and clarified.

ricked-twice and others added 2 commits June 16, 2025 09:30
@ricked-twice
Copy link
Author

Thanks for the review and the comments, I'll try to have a look at simplifying the rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants