From 8c2a6937a5b0f8eb9eaff1b69e73cd2d50fc78b4 Mon Sep 17 00:00:00 2001 From: ricked-twice Date: Fri, 20 Nov 2020 12:11:28 +0100 Subject: [PATCH 1/8] Add rule concerning raw pointers and usage of std::ptr::read --- src/en/04_language.md | 71 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/src/en/04_language.md b/src/en/04_language.md index 8491118..c8ce26a 100644 --- a/src/en/04_language.md +++ b/src/en/04_language.md @@ -204,3 +204,74 @@ the process. TODO: cyclomatic complexity of the macro expanded code, recursion limits, ... --> + +## Value displacement + +Values in Rust may have three distinct semantics when considering value displacement: + +- Either it has _move_ semantics, this behavior is the default one. +- Or it has move _move_ semantics plus the _drop_ semantics, i.e. its type implements the `Drop` trait. +- Or it has copy semantics, by having its type implementing the `Copy` trait. + +However, some problem appears when using the `std::ptr::read` function. +According to the [documentation](https://doc.rust-lang.org/std/ptr/fn.read.html), the function: +> Reads the value from src without moving it. This leaves the memory in src unchanged. + +To be clear, this function is somehow copying the value pointed by the raw pointer, regardless of its type semantics. +This is a dangerous behavior as it can lead to double-free and/or double-drop in some cases. + +To illustrate the copy on a type having the move semantics let's consider the following snippet: + +```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); +} +``` + +Here we can see that a second object is created by the call to `std::ptr::read`, i.e. a copy of a _non-copy_ object is performed. +Here, the problem is not really a huge one, except if some cleaning outside of the drop implementation is performed (as it is recommended): some sensitive data might survive within the memory. + +However, this behavior might cause some resilience issue when this function is used with a raw pointer pointing to some data allocated on the heap with move semantic as illustrated by this snippet: + +```rust +# use std::boxed::Box; +# use std::ops::Drop; +# +#[derive(Debug)] +struct MyStructBoxed(Box); + +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 ); +} +``` + +> ### Rule {{#check LANG-RAW-PTR | Avoid the use of `std::ptr::read` }} +> +> As shown above, `std::ptr::read` as side effect depending on the way the type of the raw pointer is moving through different context. +> It is preferable to use the operation of referencing/dereferencing (`&*`) to avoid those side effect. From fa895f2f3687da80ecdc44e29582cf0fe3bbad2c Mon Sep 17 00:00:00 2001 From: ricked-twice Date: Fri, 20 Nov 2020 12:12:05 +0100 Subject: [PATCH 2/8] Add rule concerning raw pointers and usage of std::ptr::read - FR --- src/fr/04_language.md | 71 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/src/fr/04_language.md b/src/fr/04_language.md index 27b52a1..83eb018 100644 --- a/src/fr/04_language.md +++ b/src/fr/04_language.md @@ -223,3 +223,74 @@ d'autres raisons. TODO : complexité cyclomatique du code macro-expansé, limites de récursion, ... --> + +## 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* de la valeur si le type implément le trait `Drop`. +- 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épemment du mode de déplacement du type en question. +Ce comportement peut être dangeureux 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. +Ici, le problème n'est pas réellement dangeureux, sauf si du nettoyage de mémoire en dehors de l'implémentation de `drop` est réalisée (tel que recommandé): des données sensibles peuvent donc persister en mémoire. + +Mais ce comportement peut causer des problèmes de résilience lors de l'utilisation de cette fonction avec un *raw pointer* pointant vers des données allouées sur le tas avec un mode de déplacement par déplacement, tel qu'illustré ici : + +```rust +# use std::boxed::Box; +# use std::ops::Drop; +# +#[derive(Debug)] +struct MyStructBoxed(Box); + +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 ); +} +``` + +> ### Règle {{#check LANG-RAW-PTR | Éviter d'utiliser `std::ptr::read`}} +> +> Comme illustrer, `std::ptr::read` peut avoir des effets de bords indésriables en fonction du mode déplacement du type pointé par le *raw pointer* source. +> Il est donc préférable d'utiliser l'opération de référencement/déréférencement (`&*`) pour les éviter. From ddf9ac4f8552bdb5f59a6b0bd430c6f523ca0b1d Mon Sep 17 00:00:00 2001 From: ricked-twice Date: Fri, 20 Nov 2020 13:49:28 +0100 Subject: [PATCH 3/8] Fixing typos and phrasing --- src/en/04_language.md | 8 ++++---- src/fr/04_language.md | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/en/04_language.md b/src/en/04_language.md index c8ce26a..89600cc 100644 --- a/src/en/04_language.md +++ b/src/en/04_language.md @@ -213,12 +213,12 @@ Values in Rust may have three distinct semantics when considering value displace - Or it has move _move_ semantics plus the _drop_ semantics, i.e. its type implements the `Drop` trait. - Or it has copy semantics, by having its type implementing the `Copy` trait. -However, some problem appears when using the `std::ptr::read` function. +However, some problem may appear when using the `std::ptr::read` function. According to the [documentation](https://doc.rust-lang.org/std/ptr/fn.read.html), the function: > Reads the value from src without moving it. This leaves the memory in src unchanged. To be clear, this function is somehow copying the value pointed by the raw pointer, regardless of its type semantics. -This is a dangerous behavior as it can lead to double-free and/or double-drop in some cases. +This is a dangerous behavior as it can lead to double-drop and, in some cases, to double-free. To illustrate the copy on a type having the move semantics let's consider the following snippet: @@ -243,7 +243,7 @@ fn main(){ } ``` -Here we can see that a second object is created by the call to `std::ptr::read`, i.e. a copy of a _non-copy_ object is performed. +We can see that a second object is created by the call to `std::ptr::read`, i.e. a copy of a _non-copy_ object is performed. Here, the problem is not really a huge one, except if some cleaning outside of the drop implementation is performed (as it is recommended): some sensitive data might survive within the memory. However, this behavior might cause some resilience issue when this function is used with a raw pointer pointing to some data allocated on the heap with move semantic as illustrated by this snippet: @@ -273,5 +273,5 @@ fn main(){ > ### Rule {{#check LANG-RAW-PTR | Avoid the use of `std::ptr::read` }} > -> As shown above, `std::ptr::read` as side effect depending on the way the type of the raw pointer is moving through different context. +> `std::ptr::read` might have undesired side effect depending on the way the type of the raw pointer is moving through different context. > It is preferable to use the operation of referencing/dereferencing (`&*`) to avoid those side effect. diff --git a/src/fr/04_language.md b/src/fr/04_language.md index 83eb018..0c08936 100644 --- a/src/fr/04_language.md +++ b/src/fr/04_language.md @@ -292,5 +292,5 @@ fn main(){ > ### Règle {{#check LANG-RAW-PTR | Éviter d'utiliser `std::ptr::read`}} > -> Comme illustrer, `std::ptr::read` peut avoir des effets de bords indésriables en fonction du mode déplacement du type pointé par le *raw pointer* source. +> `std::ptr::read` peut avoir des effets de bords indésirables en fonction du mode déplacement du type pointé par le *raw pointer* source. > Il est donc préférable d'utiliser l'opération de référencement/déréférencement (`&*`) pour les éviter. From 3529cab80f7c0e8c2fd4858b31e7fee36640c497 Mon Sep 17 00:00:00 2001 From: Philippe P <39213807+ricked-twice@users.noreply.github.com> Date: Mon, 23 Nov 2020 09:45:28 +0100 Subject: [PATCH 4/8] Apply suggestions from code review - EN Co-authored-by: Daniel Henry-Mantilla --- src/en/04_language.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/en/04_language.md b/src/en/04_language.md index 89600cc..48a472d 100644 --- a/src/en/04_language.md +++ b/src/en/04_language.md @@ -210,14 +210,14 @@ limits, ... Values in Rust may have three distinct semantics when considering value displacement: - Either it has _move_ semantics, this behavior is the default one. -- Or it has move _move_ semantics plus the _drop_ semantics, i.e. its type implements the `Drop` trait. +- Or it has move _move_ semantics plus the _drop_ semantics, i.e. its type has [_drop glue_](https://docs.rs/core/mem/fn.needs_drop.html) (by directly implementing `Drop` or by containing a field with _drop glue_). - Or it has copy semantics, by having its type implementing the `Copy` trait. However, some problem may appear when using the `std::ptr::read` function. According to the [documentation](https://doc.rust-lang.org/std/ptr/fn.read.html), the function: > Reads the value from src without moving it. This leaves the memory in src unchanged. -To be clear, this function is somehow copying the value pointed by the raw pointer, regardless of its type semantics. +To be clear, this function is performing a bit-wise (shallow) copy of the pointee (value pointed to by the raw pointer), regardless of its type semantics. This is a dangerous behavior as it can lead to double-drop and, in some cases, to double-free. To illustrate the copy on a type having the move semantics let's consider the following snippet: From b5c8d80bc723e0e899a9bb9d5a50333a0b990213 Mon Sep 17 00:00:00 2001 From: ricked-twice Date: Mon, 23 Nov 2020 10:13:50 +0100 Subject: [PATCH 5/8] Fixing URL --- src/en/04_language.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/en/04_language.md b/src/en/04_language.md index 48a472d..055dcdb 100644 --- a/src/en/04_language.md +++ b/src/en/04_language.md @@ -210,7 +210,7 @@ limits, ... Values in Rust may have three distinct semantics when considering value displacement: - Either it has _move_ semantics, this behavior is the default one. -- Or it has move _move_ semantics plus the _drop_ semantics, i.e. its type has [_drop glue_](https://docs.rs/core/mem/fn.needs_drop.html) (by directly implementing `Drop` or by containing a field with _drop glue_). +- Or it has move _move_ semantics plus the _drop_ semantics, i.e. its type has [_drop glue_](https://doc.rust-lang.org/core/mem/fn.needs_drop.html) (by directly implementing `Drop` or by containing a field with _drop glue_). - Or it has copy semantics, by having its type implementing the `Copy` trait. However, some problem may appear when using the `std::ptr::read` function. From ba1bc8a51a669f3313b8bd598a9ca8703122f827 Mon Sep 17 00:00:00 2001 From: ricked-twice Date: Mon, 23 Nov 2020 10:15:39 +0100 Subject: [PATCH 6/8] Fixing typos - (FR) --- src/fr/04_language.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/fr/04_language.md b/src/fr/04_language.md index 0c08936..7584a0f 100644 --- a/src/fr/04_language.md +++ b/src/fr/04_language.md @@ -229,15 +229,15 @@ récursion, ... 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* de la valeur si le type implément le trait `Drop`. +- Ou par *déplacement* plus un *drop* de la valeur si le type implémente le trait `Drop`. - 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épemment du mode de déplacement du type en question. -Ce comportement peut être dangeureux car il peut mener à des *double-free* et/ou des *double-drop*. +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 : @@ -263,7 +263,7 @@ fn main(){ ``` 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. -Ici, le problème n'est pas réellement dangeureux, sauf si du nettoyage de mémoire en dehors de l'implémentation de `drop` est réalisée (tel que recommandé): des données sensibles peuvent donc persister en mémoire. +Ici, le problème n'est pas réellement dangereux, sauf si du nettoyage de mémoire en dehors de l'implémentation de `drop` est réalisée (tel que recommandé): des données sensibles peuvent donc persister en mémoire. Mais ce comportement peut causer des problèmes de résilience lors de l'utilisation de cette fonction avec un *raw pointer* pointant vers des données allouées sur le tas avec un mode de déplacement par déplacement, tel qu'illustré ici : From 4716c7fa7b120790ba6ff65b18fb859424e6110e Mon Sep 17 00:00:00 2001 From: ricked-twice Date: Mon, 23 Nov 2020 10:28:44 +0100 Subject: [PATCH 7/8] Add examples output --- src/en/04_language.md | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/en/04_language.md b/src/en/04_language.md index 055dcdb..6d12173 100644 --- a/src/en/04_language.md +++ b/src/en/04_language.md @@ -237,11 +237,21 @@ impl Drop for MyStruct { } fn main(){ - let obj: MyStruct = MyStruct(100); - let ptr: *const MyStruct = &test as *const MyStruct; + let obj: MyStruct = MyStruct(42); + let ptr: *const MyStruct = &obj as *const MyStruct; println!("{:?} @ {:p}", unsafe { std::ptr::read(ptr) }, ptr); } ``` +Output (may vary): +``` +MyStruct(42) @ 0x7ffcb01ec737 +---Dropping an object--- +Before zeroing: 42 @ 0x7ffcb01ec7a7 +After zeroing: 0 @ 0x7ffcb01ec7a7 +Before zeroing: 42 @ 0x7ffcb01ec737 +---Dropping an object--- +After zeroing: 0 @ 0x7ffcb01ec737 +``` We can see that a second object is created by the call to `std::ptr::read`, i.e. a copy of a _non-copy_ object is performed. Here, the problem is not really a huge one, except if some cleaning outside of the drop implementation is performed (as it is recommended): some sensitive data might survive within the memory. @@ -265,11 +275,20 @@ impl Drop for MyStructBoxed { } fn main(){ - let test: MyStructBoxed = MyStructBoxed(Box::new(100)); - let ptr: *const MyStructBoxed = &test as *const MyStructBoxed; + let obj: MyStructBoxed = MyStructBoxed(Box::new(42)); + let ptr: *const MyStructBoxed = &obj as *const MyStructBoxed; println!("{:?} @ {:p}", unsafe { std::ptr::read(ptr) }, unsafe { &*ptr }.0 ); } ``` +Output (may vary): +``` +MyStructBoxed(42) @ 0x55fe454b3c10 +Before zeroing: 42 @ 0x55fe454b3c10 +After zeroing: 0 @ 0x55fe454b3c10 +Before zeroing: 179 @ 0x55fe454b3c10 +After zeroing: 0 @ 0x55fe454b3c10 +free(): double free detected in tcache 2 +``` > ### Rule {{#check LANG-RAW-PTR | Avoid the use of `std::ptr::read` }} > From 4fcfeb4506a948554a1dfd4e4b85f3bb97bb57dc Mon Sep 17 00:00:00 2001 From: ricked-twice <39213807+ricked-twice@users.noreply.github.com> Date: Fri, 20 May 2022 19:06:21 +0200 Subject: [PATCH 8/8] (Finally) Taking last reviews into accounts --- src/en/04_language.md | 29 ++++++++++++++++++++++++++--- src/fr/04_language.md | 30 +++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/en/04_language.md b/src/en/04_language.md index 6d12173..a213031 100644 --- a/src/en/04_language.md +++ b/src/en/04_language.md @@ -254,10 +254,31 @@ After zeroing: 0 @ 0x7ffcb01ec737 ``` We can see that a second object is created by the call to `std::ptr::read`, i.e. a copy of a _non-copy_ object is performed. -Here, the problem is not really a huge one, except if some cleaning outside of the drop implementation is performed (as it is recommended): some sensitive data might survive within the memory. - -However, this behavior might cause some resilience issue when this function is used with a raw pointer pointing to some data allocated on the heap with move semantic as illustrated by this snippet: +This causes problems of varying severity: + + - In some _rare_ cases (no _drop glue_, no non-aliasing guarantees, no _safety invariants_ attached to the type; basically when the type should have been `Copy` itself), it can be harmless. + + - In most cases, there may be _safety_ or _validity_ invariants (such as pointer aliasing, or rather, lack thereof) which make one instance become `unsafe` to use, or even UB to use, the moment the other instance is. + + - Example: + + ```rust,no_run + # use ::core::mem; + # + let box_1: Box = Box::new(42); + let at_box_1: *const Box = &box_1; + let box_2: Box = 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. + ``` + See [Stacked Borrows](https://plv.mpi-sws.org/rustbelt/stacked-borrows/) for more info. + + - If the type has drop glue, then, when one of the two instances is dropped, any usage of the other may cause a use-after-free, the most notable example being that the second instance will be, in and of itself, freed, causing a double-free. + + **This is Undefined Behavior, and is a source of major vulnerabilities**. + +The following snippet showcases this double-free bug: ```rust # use std::boxed::Box; # use std::ops::Drop; @@ -294,3 +315,5 @@ free(): double free detected in tcache 2 > > `std::ptr::read` might have undesired side effect depending on the way the type of the raw pointer is moving through different context. > It is preferable to use the operation of referencing/dereferencing (`&*`) to avoid those side effect. +> +> Moreover, if pointee `T` is `Copy`, the `*`-dereference operator should be used. diff --git a/src/fr/04_language.md b/src/fr/04_language.md index 7584a0f..663512c 100644 --- a/src/fr/04_language.md +++ b/src/fr/04_language.md @@ -229,7 +229,7 @@ récursion, ... 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* de la valeur si le type implémente le trait `Drop`. +- Ou par *déplacement* plus un *drop* si `mem::needs_drop::()` 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`. @@ -263,9 +263,31 @@ fn main(){ ``` 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. -Ici, le problème n'est pas réellement dangereux, sauf si du nettoyage de mémoire en dehors de l'implémentation de `drop` est réalisée (tel que recommandé): des données sensibles peuvent donc persister en mémoire. -Mais ce comportement peut causer des problèmes de résilience lors de l'utilisation de cette fonction avec un *raw pointer* pointant vers des données allouées sur le tas avec un mode de déplacement par déplacement, tel qu'illustré ici : +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 = Box::new(42); + let at_box_1: *const Box = &box_1; + let box_2: Box = 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::()` 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; @@ -294,3 +316,5 @@ fn main(){ > > `std::ptr::read` peut avoir des effets de bords indésirables en fonction du mode déplacement du type pointé par le *raw pointer* source. > Il est donc préférable d'utiliser l'opération de référencement/déréférencement (`&*`) pour les éviter. +> +> De plus, si le la valeur `T` pointée est `Copy`, l'opérateur de `*`-déréférencement doit être utilisé.