mirror of
https://gitlab.torproject.org/tpo/core/tor.git
synced 2024-11-14 15:23:27 +01:00
470 lines
18 KiB
Markdown
470 lines
18 KiB
Markdown
|
||
Rust Coding Standards
|
||
=======================
|
||
|
||
You MUST follow the standards laid out in `.../doc/HACKING/CodingStandards.md`,
|
||
where applicable.
|
||
|
||
Module/Crate Declarations
|
||
---------------------------
|
||
|
||
Each Tor C module which is being rewritten MUST be in its own crate.
|
||
See the structure of `.../src/rust` for examples.
|
||
|
||
In your crate, you MUST use `lib.rs` ONLY for pulling in external
|
||
crates (e.g. `extern crate libc;`) and exporting public objects from
|
||
other Rust modules (e.g. `pub use mymodule::foo;`). For example, if
|
||
you create a crate in `.../src/rust/yourcrate`, your Rust code should
|
||
live in `.../src/rust/yourcrate/yourcode.rs` and the public interface
|
||
to it should be exported in `.../src/rust/yourcrate/lib.rs`.
|
||
|
||
If your code is to be called from Tor C code, you MUST define a safe
|
||
`ffi.rs`. See the "Safety" section further down for more details.
|
||
|
||
For example, in a hypothetical `tor_addition` Rust module:
|
||
|
||
In `.../src/rust/tor_addition/addition.rs`:
|
||
|
||
pub fn get_sum(a: i32, b: i32) -> i32 {
|
||
a + b
|
||
}
|
||
|
||
In `.../src/rust/tor_addition/lib.rs`:
|
||
|
||
pub use addition::*;
|
||
|
||
In `.../src/rust/tor_addition/ffi.rs`:
|
||
|
||
#[no_mangle]
|
||
pub extern "C" fn tor_get_sum(a: c_int, b: c_int) -> c_int {
|
||
get_sum(a, b)
|
||
}
|
||
|
||
If your Rust code must call out to parts of Tor's C code, you must
|
||
declare the functions you are calling in the `external` crate, located
|
||
at `.../src/rust/external`.
|
||
|
||
<!-- XXX get better examples of how to declare these externs, when/how they -->
|
||
<!-- XXX are unsafe, what they are expected to do —isis -->
|
||
|
||
Modules should strive to be below 500 lines (tests excluded). Single
|
||
responsibility and limited dependencies should be a guiding standard.
|
||
|
||
If you have any external modules as dependencies (e.g. `extern crate
|
||
libc;`), you MUST declare them in your crate's `lib.rs` and NOT in any
|
||
other module.
|
||
|
||
Dependencies
|
||
--------------
|
||
|
||
In general, we use modules from only the Rust standard library
|
||
whenever possible. We will review including external crates on a
|
||
case-by-case basis.
|
||
|
||
Documentation
|
||
---------------
|
||
|
||
You MUST include `#[deny(missing_docs)]` in your crate.
|
||
|
||
For function/method comments, you SHOULD include a one-sentence, "first person"
|
||
description of function behaviour (see requirements for documentation as
|
||
described in `.../src/HACKING/CodingStandards.md`), then an `# Inputs` section
|
||
for inputs or initialisation values, a `# Returns` section for return
|
||
values/types, a `# Warning` section containing warnings for unsafe behaviours or
|
||
panics that could happen. For publicly accessible
|
||
types/constants/objects/functions/methods, you SHOULD also include an
|
||
`# Examples` section with runnable doctests.
|
||
|
||
You MUST document your module with _module docstring_ comments,
|
||
i.e. `//!` at the beginning of each line.
|
||
|
||
Style
|
||
-------
|
||
|
||
You SHOULD consider breaking up large literal numbers with `_` when it makes it
|
||
more human readable to do so, e.g. `let x: u64 = 100_000_000_000`.
|
||
|
||
Testing
|
||
---------
|
||
|
||
All code MUST be unittested and integration tested.
|
||
|
||
Public functions/objects exported from a crate SHOULD include doctests
|
||
describing how the function/object is expected to be used.
|
||
|
||
Integration tests SHOULD go into a `tests/` directory inside your
|
||
crate. Unittests SHOULD go into their own module inside the module
|
||
they are testing, e.g. in `.../src/rust/tor_addition/addition.rs` you
|
||
should put:
|
||
|
||
#[cfg(test)]
|
||
mod test {
|
||
use super::*;
|
||
|
||
#[test]
|
||
fn addition_with_zero() {
|
||
let sum: i32 = get_sum(5i32, 0i32);
|
||
assert_eq!(sum, 5);
|
||
}
|
||
}
|
||
|
||
Benchmarking
|
||
--------------
|
||
|
||
The external `test` crate can be used for most benchmarking. However, using
|
||
this crate requires nightly Rust. Since we may want to switch to a more
|
||
stable Rust compiler eventually, we shouldn't do things which will automatically
|
||
break builds for stable compilers. Therefore, you MUST feature-gate your
|
||
benchmarks in the following manner.
|
||
|
||
If you wish to benchmark some of your Rust code, you MUST put the
|
||
following in the `[features]` section of your crate's `Cargo.toml`:
|
||
|
||
[features]
|
||
bench = []
|
||
|
||
Next, in your crate's `lib.rs` you MUST put:
|
||
|
||
#[cfg(all(test, feature = "bench"))]
|
||
extern crate test;
|
||
|
||
This ensures that the external crate `test`, which contains utilities
|
||
for basic benchmarks, is only used when running benchmarks via `cargo
|
||
bench --features bench`.
|
||
|
||
Finally, to write your benchmark code, in
|
||
`.../src/rust/tor_addition/addition.rs` you SHOULD put:
|
||
|
||
#[cfg(all(test, features = "bench"))]
|
||
mod bench {
|
||
use test::Bencher;
|
||
use super::*;
|
||
|
||
#[bench]
|
||
fn addition_small_integers(b: &mut Bencher) {
|
||
b.iter(| | get_sum(5i32, 0i32));
|
||
}
|
||
}
|
||
|
||
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).
|
||
|
||
Whitespace & Formatting
|
||
-------------------------
|
||
|
||
You MUST run `rustfmt` (https://github.com/rust-lang-nursery/rustfmt)
|
||
on your code before your code will be merged. You can install rustfmt
|
||
by doing `cargo install rustfmt-nightly` and then run it with `cargo
|
||
fmt`.
|
||
|
||
Safety
|
||
--------
|
||
|
||
You SHOULD read [the nomicon](https://doc.rust-lang.org/nomicon/) before writing
|
||
Rust FFI code. It is *highly advised* that you read and write normal Rust code
|
||
before attempting to write FFI or any other unsafe code.
|
||
|
||
Here are some additional bits of advice and rules:
|
||
|
||
0. Any behaviours which Rust considers to be undefined are forbidden
|
||
|
||
From https://doc.rust-lang.org/reference/behavior-considered-undefined.html:
|
||
|
||
> Behavior considered undefined
|
||
>
|
||
> The following is a list of behavior which is forbidden in all Rust code,
|
||
> including within unsafe blocks and unsafe functions. Type checking provides the
|
||
> guarantee that these issues are never caused by safe code.
|
||
>
|
||
> * Data races
|
||
> * Dereferencing a null/dangling raw pointer
|
||
> * Reads of [undef](http://llvm.org/docs/LangRef.html#undefined-values)
|
||
> (uninitialized) memory
|
||
> * Breaking the
|
||
> [pointer aliasing rules](http://llvm.org/docs/LangRef.html#pointer-aliasing-rules)
|
||
> with raw pointers (a subset of the rules used by C)
|
||
> * `&mut T` and `&T` follow LLVM’s scoped noalias model, except if the `&T`
|
||
> contains an `UnsafeCell<U>`. Unsafe code must not violate these aliasing
|
||
> guarantees.
|
||
> * Mutating non-mutable data (that is, data reached through a shared
|
||
> reference or data owned by a `let` binding), unless that data is
|
||
> contained within an `UnsafeCell<U>`.
|
||
> * Invoking undefined behavior via compiler intrinsics:
|
||
> - Indexing outside of the bounds of an object with
|
||
> `std::ptr::offset` (`offset` intrinsic), with the exception of
|
||
> one byte past the end which is permitted.
|
||
> - Using `std::ptr::copy_nonoverlapping_memory` (`memcpy32`/`memcpy64`
|
||
> intrinsics) on overlapping buffers
|
||
> * Invalid values in primitive types, even in private fields/locals:
|
||
> - Dangling/null references or boxes
|
||
> - A value other than `false` (0) or `true` (1) in a `bool`
|
||
> - A discriminant in an `enum` not included in the type definition
|
||
> - A value in a `char` which is a surrogate or above `char::MAX`
|
||
> - Non-UTF-8 byte sequences in a `str`
|
||
> * Unwinding into Rust from foreign code or unwinding from Rust into foreign
|
||
> code. Rust's failure system is not compatible with exception handling in other
|
||
> languages. Unwinding must be caught and handled at FFI boundaries.
|
||
|
||
1. `unwrap()`
|
||
|
||
If you call `unwrap()`, anywhere, even in a test, you MUST include
|
||
an inline comment stating how the unwrap will either 1) never fail,
|
||
or 2) should fail (i.e. in a unittest).
|
||
|
||
You SHOULD NOT use `unwrap()` anywhere in which it is possible to handle the
|
||
potential error with either `expect()` or the eel operator, `?`.
|
||
For example, consider a function which parses a string into an integer:
|
||
|
||
fn parse_port_number(config_string: &str) -> u16 {
|
||
u16::from_str_radix(config_string, 10).unwrap()
|
||
}
|
||
|
||
There are numerous ways this can fail, and the `unwrap()` will cause the
|
||
whole program to byte the dust! Instead, either you SHOULD use `expect()`
|
||
(or another equivalent function which will return an `Option` or a `Result`)
|
||
and change the return type to be compatible:
|
||
|
||
fn parse_port_number(config_string: &str) -> Option<u16> {
|
||
u16::from_str_radix(config_string, 10).expect("Couldn't parse port into a u16")
|
||
}
|
||
|
||
or you SHOULD use `or()` (or another similar method):
|
||
|
||
fn parse_port_number(config_string: &str) -> Option<u16> {
|
||
u16::from_str_radix(config_string, 10).or(Err("Couldn't parse port into a u16")
|
||
}
|
||
|
||
Using methods like `or()` can be particularly handy when you must do
|
||
something afterwards with the data, for example, if we wanted to guarantee
|
||
that the port is high. Combining these methods with the eel operator (`?`)
|
||
makes this even easier:
|
||
|
||
fn parse_port_number(config_string: &str) -> Result<u16, Err> {
|
||
let port = u16::from_str_radix(config_string, 10).or(Err("Couldn't parse port into a u16"))?;
|
||
|
||
if port > 1024 {
|
||
return Ok(port);
|
||
} else {
|
||
return Err("Low ports not allowed");
|
||
}
|
||
}
|
||
|
||
2. `unsafe`
|
||
|
||
If you use `unsafe`, you MUST describe a contract in your
|
||
documentation which describes how and when the unsafe code may
|
||
fail, and what expectations are made w.r.t. the interfaces to
|
||
unsafe code. This is also REQUIRED for major pieces of FFI between
|
||
C and Rust.
|
||
|
||
When creating an FFI in Rust for C code to call, it is NOT REQUIRED
|
||
to declare the entire function `unsafe`. For example, rather than doing:
|
||
|
||
#[no_mangle]
|
||
pub unsafe extern "C" fn increment_and_combine_numbers(mut numbers: [u8; 4]) -> u32 {
|
||
for number in &mut numbers {
|
||
*number += 1;
|
||
}
|
||
std::mem::transmute::<[u8; 4], u32>(numbers)
|
||
}
|
||
|
||
You SHOULD instead do:
|
||
|
||
#[no_mangle]
|
||
pub extern "C" fn increment_and_combine_numbers(mut numbers: [u8; 4]) -> u32 {
|
||
for index in 0..numbers.len() {
|
||
numbers[index] += 1;
|
||
}
|
||
unsafe {
|
||
std::mem::transmute::<[u8; 4], u32>(numbers)
|
||
}
|
||
}
|
||
|
||
3. Pass only integer types and bytes over the boundary
|
||
|
||
The only non-integer type which may cross the FFI boundary is
|
||
bytes, e.g. `&[u8]`. This SHOULD be done on the Rust side by
|
||
passing a pointer (`*mut libc::c_char`) and a length
|
||
(`libc::size_t`).
|
||
|
||
One might be tempted to do this via doing
|
||
`CString::new("blah").unwrap().into_raw()`. This has several problems:
|
||
|
||
a) If you do `CString::new("bl\x00ah")` then the unwrap() will fail
|
||
due to the additional NULL terminator, causing a dangling
|
||
pointer to be returned (as well as a potential use-after-free).
|
||
|
||
b) Returning the raw pointer will cause the CString to run its deallocator,
|
||
which causes any C code which tries to access the contents to dereference a
|
||
NULL pointer.
|
||
|
||
c) If we were to do `as_raw()` this would result in a potential double-free
|
||
since the Rust deallocator would run and possibly Tor's deallocator.
|
||
|
||
d) Calling `into_raw()` without later using the same pointer in Rust to call
|
||
`from_raw()` and then deallocate in Rust can result in a
|
||
[memory leak](https://doc.rust-lang.org/std/ffi/struct.CString.html#method.into_raw).
|
||
|
||
[It was determined](https://github.com/rust-lang/rust/pull/41074) that this
|
||
is safe to do if you use the same allocator in C and Rust and also specify
|
||
the memory alignment for CString (except that there is no way to specify
|
||
the alignment for CString). It is believed that the alignment is always 1,
|
||
which would mean it's safe to dealloc the resulting `*mut c_char` in Tor's
|
||
C code. However, the Rust developers are not willing to guarantee the
|
||
stability of, or a contract for, this behaviour, citing concerns that this
|
||
is potentially extremely and subtly unsafe.
|
||
|
||
4. Perform an allocation on the other side of the boundary
|
||
|
||
After crossing the boundary, the other side MUST perform an
|
||
allocation to copy the data and is therefore responsible for
|
||
freeing that memory later.
|
||
|
||
5. No touching other language's enums
|
||
|
||
Rust enums should never be touched from C (nor can they be safely
|
||
`#[repr(C)]`) nor vice versa:
|
||
|
||
> "The chosen size is the default enum size for the target platform's C
|
||
> ABI. Note that enum representation in C is implementation defined, so this is
|
||
> really a "best guess". In particular, this may be incorrect when the C code
|
||
> of interest is compiled with certain flags."
|
||
|
||
(from https://gankro.github.io/nomicon/other-reprs.html)
|
||
|
||
6. Type safety
|
||
|
||
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.
|
||
|
||
9. Casting integers with `as`
|
||
|
||
This is generally fine to do, but it has some behaviours which you should be
|
||
aware of. Casting down chops off the high bits, e.g.:
|
||
|
||
let x: u32 = 4294967295;
|
||
println!("{}", x as u16); // prints 65535
|
||
|
||
Some cases which you MUST NOT do include:
|
||
|
||
* Casting an `u128` down to an `f32` or vice versa (e.g.
|
||
`u128::MAX as f32` but this isn't only a problem with overflowing
|
||
as it is also undefined behaviour for `42.0f32 as u128`),
|
||
|
||
* Casting between integers and floats when the thing being cast
|
||
cannot fit into the type it is being casted into, e.g.:
|
||
|
||
println!("{}", 42949.0f32 as u8); // prints 197 in debug mode and 0 in release
|
||
println!("{}", 1.04E+17 as u8); // prints 0 in both modes
|
||
println!("{}", (0.0/0.0) as i64); // prints whatever the heck LLVM wants
|
||
|
||
Because this behaviour is undefined, it can even produce segfaults in
|
||
safe Rust code. For example, the following program built in release
|
||
mode segfaults:
|
||
|
||
#[inline(never)]
|
||
pub fn trigger_ub(sl: &[u8; 666]) -> &[u8] {
|
||
// Note that the float is out of the range of `usize`, invoking UB when casting.
|
||
let idx = 1e99999f64 as usize;
|
||
&sl[idx..] // The bound check is elided due to `idx` being of an undefined value.
|
||
}
|
||
|
||
fn main() {
|
||
println!("{}", trigger_ub(&[1; 666])[999999]); // ~ out of bound
|
||
}
|
||
|
||
And in debug mode panics with:
|
||
|
||
thread 'main' panicked at 'slice index starts at 140721821254240 but ends at 666', /checkout/src/libcore/slice/mod.rs:754:4
|