[tor-commits] [tor/master] docs: More Rust coding standards w.r.t. fuzzing and safety.

[nickm at torproject.org]

commit aeef8a093f517bf8652a7ee9f46429f5e5b33d22
Author: Isis Lovecruft <isis at torproject.org>
Date:   Wed Aug 30 21:54:41 2017 +0000

    docs: More Rust coding standards w.r.t. fuzzing and safety.
---
 doc/HACKING/CodingStandardsRust.md | 100 +++++++++++++++++++++++++++++++++++--
 1 file changed, 97 insertions(+), 3 deletions(-)

diff --git a/doc/HACKING/CodingStandardsRust.md b/doc/HACKING/CodingStandardsRust.md
index 41b76263a..fc4977ed9 100644
--- a/doc/HACKING/CodingStandardsRust.md
+++ b/doc/HACKING/CodingStandardsRust.md
@@ -140,6 +140,13 @@ Finally, to write your benchmark code, in
         }
     }
 
+ Fuzzing
+---------
+
+If you wish to fuzz parts of your code, please see the
+[`cargo fuzz`](https://github.com/rust-fuzz/cargo-fuzz) crate, which uses
+[libfuzzer-sys](https://github.com/rust-fuzz/libfuzzer-sys).
+
  Safety
 --------
 
@@ -241,9 +248,96 @@ Here are some additional bits of advice and rules:
 
 6. Type safety
 
-   Wherever possible and sensical, you SHOULD create new types, either as tuple
-   structs (e.g. `struct MyInteger(pub u32)`) or as type aliases (e.g. `pub type
-   MyInteger = u32`).
+   Wherever possible and sensical, you SHOULD create new types in a
+   manner which prevents type confusion or misuse.  For example,
+   rather than using an untyped mapping between strings and integers
+   like so:
+
+        use std::collections::HashMap;
+
+        pub fn get_elements_with_over_9000_points(map: &HashMap<String, usize>) -> Vec<String> {
+            ...
+        }
+
+   It would be safer to define a new type, such that some other usage
+   of `HashMap<String, usize>` cannot be confused for this type:
+
+        pub struct DragonBallZPowers(pub HashMap<String, usize>);
+
+        impl DragonBallZPowers {
+            pub fn over_nine_thousand<'a>(&'a self) -> Vec<&'a String> {
+                let mut powerful_enough: Vec<&'a String> = Vec::with_capacity(5);
+
+                for (character, power) in &self.0 {
+                    if *power > 9000 {
+                        powerful_enough.push(character);
+                    }
+                }
+                powerful_enough
+            }
+        }
+
+   Note the following code, which uses Rust's type aliasing, is valid
+   but it does NOT meet the desired type safety goals:
+
+        pub type Power = usize;
+
+        pub fn over_nine_thousand(power: &Power) -> bool {
+            if *power > 9000 {
+                return true;
+            }
+            false
+        }
+
+        // We can still do the following:
+        let his_power: usize = 9001;
+        over_nine_thousand(&his_power);
+
+7. Unsafe mucking around with lifetimes
+
+   Because lifetimes are technically, in type theory terms, a kind, i.e. a
+   family of types, individual lifetimes can be treated as types.  For example,
+   one can arbitrarily extend and shorten lifetime using `std::mem::transmute`:
+
+        struct R<'a>(&'a i32);
+
+        unsafe fn extend_lifetime<'b>(r: R<'b>) -> R<'static> {
+            std::mem::transmute::<R<'b>, R<'static>>(r)
+        }
+
+        unsafe fn shorten_invariant_lifetime<'b, 'c>(r: &'b mut R<'static>) -> &'b mut R<'c> {
+            std::mem::transmute::<&'b mut R<'static>, &'b mut R<'c>>(r)
+        }
+
+   Calling `extend_lifetime()` would cause an `R` passed into it to live forever
+   for the life of the program (the `'static` lifetime).  Similarly,
+   `shorten_invariant_lifetime()` could be used to take something meant to live
+   forever, and cause it to disappear!  This is incredibly unsafe.  If you're
+   going to be mucking around with lifetimes like this, first, you better have
+   an extremely good reason, and second, you may as be honest and explicit about
+   it, and for ferris' sake just use a raw pointer.
+
+   In short, just because lifetimes can be treated like types doesn't mean you
+   should do it.
+
+8. Doing excessively unsafe things when there's a safer alternative
+
+   Similarly to #7, often there are excessively unsafe ways to do a task and a
+   simpler, safer way.  You MUST choose the safer option where possible.
+
+   For example, `std::mem::transmute` can be abused in ways where casting with
+   `as` would be both simpler and safer:
+
+        // Don't do this
+        let ptr = &0;
+        let ptr_num_transmute = unsafe { std::mem::transmute::<&i32, usize>(ptr)};
+
+        // Use an `as` cast instead
+        let ptr_num_cast = ptr as *const i32 as usize;
+
+   In fact, using `std::mem::transmute` for *any* reason is a code smell and as
+   such SHOULD be avoided.
+
 
  Whitespace & Formatting
 -------------------------