diff options
Diffstat (limited to 'doc/HACKING')
| -rw-r--r-- | doc/HACKING/CodingStandards.md | 147 | ||||
| -rw-r--r-- | doc/HACKING/CodingStandardsRust.md | 469 | ||||
| -rw-r--r-- | doc/HACKING/GettingStarted.md | 5 | ||||
| -rw-r--r-- | doc/HACKING/GettingStartedRust.md | 161 | ||||
| -rw-r--r-- | doc/HACKING/HelpfulTools.md | 22 | ||||
| -rw-r--r-- | doc/HACKING/HowToReview.md | 3 | ||||
| -rw-r--r-- | doc/HACKING/ReleasingTor.md | 33 | ||||
| -rw-r--r-- | doc/HACKING/WritingTests.md | 6 |
8 files changed, 820 insertions, 26 deletions
diff --git a/doc/HACKING/CodingStandards.md b/doc/HACKING/CodingStandards.md index c7787a72cc..dd21d6fd2c 100644 --- a/doc/HACKING/CodingStandards.md +++ b/doc/HACKING/CodingStandards.md @@ -6,8 +6,8 @@ tl;dr: - Run configure with `--enable-fatal-warnings` - Document your functions - Write unit tests - - Run `make test-full` to test against all unit and integration tests. - - Run `make distcheck` to ensure the distribution works + - Run `make check` before submitting a patch + - Run `make distcheck` if you have made changes to build system components - Add a file in `changes` for your branch. Patch checklist @@ -32,6 +32,16 @@ Did you remember... - To base your code on the appropriate branch? - To include a file in the `changes` directory as appropriate? +If you are submitting a major patch or new feature, or want to in the future... + + - Set up Chutney and Stem, see HACKING/WritingTests.md + - Run `make test-full` to test against all unit and integration tests. + +If you have changed build system components: + - Please run `make distcheck` + - For example, if you have changed Makefiles, autoconf files, or anything + else that affects the build system. + How we use Git branches ======================= @@ -52,8 +62,17 @@ before it gets merged into maint, but that's rare. If you're working on a bugfix for a bug that occurs in a particular version, base your bugfix branch on the "maint" branch for the first supported series -that has that bug. (As of June 2013, we're supporting 0.2.3 and later.) If -you're working on a new feature, base it on the master branch. +that has that bug. (As of June 2013, we're supporting 0.2.3 and later.) + +If you're working on a new feature, base it on the master branch. If you're +working on a new feature and it will take a while to implement and/or you'd +like to avoid the possibility of unrelated bugs in Tor while you're +implementing your feature, consider branching off of the latest maint- branch. +_Never_ branch off a relase- branch. Don't branch off a tag either: they come +from release branches. Doing so will likely produce a nightmare of merge +conflicts in the ChangeLog when it comes time to merge your branch into Tor. +Best advice: don't try to keep an independent branch forked for more than 6 +months and expect it to merge cleanly. Try to merge pieces early and often. How we log changes @@ -77,17 +96,34 @@ you can use `git describe --contains <sha1 of commit>`. If at all possible, try to create this file in the same commit where you are making the change. Please give it a distinctive name that no other branch will use for the lifetime of your change. To verify the format of the changes file, -you can use `make check-changes`. +you can use `make check-changes`. This is run automatically as part of +`make check` -- if it fails, we must fix it before we release. These +checks are implemented in `scripts/maint/lintChanges.py`. + +Changes file style guide: + * Changes files begin with " o Header (subheading):". The header + should usually be "Minor/Major bugfixes/features". The subheading is a + particular area within Tor. See the ChangeLog for examples. + + * Make everything terse. + + * Write from the user's point of view: describe the user-visible changes + right away. + + * Mention configuration options by name. If they're rare or unusual, + remind people what they're for. + + * Describe changes in the present tense and in the imperative: not past. + + * Every bugfix should have a sentence of the form "Fixes bug 1234; bugfix + on 0.1.2.3-alpha", describing what bug was fixed and where it came from. + + * "Relays", not "servers", "nodes", or "Tor relays". When we go to make a release, we will concatenate all the entries in changes to make a draft changelog, and clear the directory. We'll then edit the draft changelog into a nice readable format. -To make sure that stuff is in the right format, we use -scripts/maint/lintChanges.py to check the changes files for -(superficial) validity. You can run this script on your own changes -files! - What needs a changes file? * A not-exhaustive list: Anything that might change user-visible @@ -156,6 +192,79 @@ old C functions. Use `strlcat`, `strlcpy`, or `tor_snprintf/tor_asprintf` inste We don't call `memcmp()` directly. Use `fast_memeq()`, `fast_memneq()`, `tor_memeq()`, or `tor_memneq()` for most purposes. +Also see a longer list of functions to avoid in: +https://people.torproject.org/~nickm/tor-auto/internal/this-not-that.html + +Floating point math is hard +--------------------------- + +Floating point arithmetic as typically implemented by computers is +very counterintuitive. Failure to adequately analyze floating point +usage can result in surprising behavior and even security +vulnerabilities! + +General advice: + + - Don't use floating point. + - If you must use floating point, document how the limits of + floating point precision and calculation accuracy affect function + outputs. + - Try to do as much as possible of your calculations using integers + (possibly acting as fixed-point numbers) and convert to floating + point for display. + - If you must send floating point numbers on the wire, serialize + them in a platform-independent way. Tor avoids exchanging + floating-point values, but when it does, it uses ASCII numerals, + with a decimal point ("."). + - Binary fractions behave very differently from decimal fractions. + Make sure you understand how these differences affect your + calculations. + - Every floating point arithmetic operation is an opportunity to + lose precision, overflow, underflow, or otherwise produce + undesired results. Addition and subtraction tend to be worse + than multiplication and division (due to things like catastrophic + cancellation). Try to arrange your calculations to minimize such + effects. + - Changing the order of operations changes the results of many + floating-point calculations. Be careful when you simplify + calculations! If the order is significant, document it using a + code comment. + - Comparing most floating point values for equality is unreliable. + Avoid using `==`, instead, use `>=` or `<=`. If you use an + epsilon value, make sure it's appropriate for the ranges in + question. + - Different environments (including compiler flags and per-thread + state on a single platform!) can get different results from the + same floating point calculations. This means you can't use + floats in anything that needs to be deterministic, like consensus + generation. This also makes reliable unit tests of + floating-point outputs hard to write. + +For additional useful advice (and a little bit of background), see +[What Every Programmer Should Know About Floating-Point +Arithmetic](http://floating-point-gui.de/). + +A list of notable (and surprising) facts about floating point +arithmetic is at [Floating-point +complexities](https://randomascii.wordpress.com/2012/04/05/floating-point-complexities/). +Most of that [series of posts on floating +point](https://randomascii.wordpress.com/category/floating-point/) is +helpful. + +For more detailed (and math-intensive) background, see [What Every +Computer Scientist Should Know About Floating-Point +Arithmetic](https://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html). + +Other C conventions +------------------- + +The `a ? b : c` trinary operator only goes inside other expressions; +don't use it as a replacement for if. (You can ignore this inside macro +definitions when necessary.) + +Assignment operators shouldn't nest inside other expressions. (You can +ignore this inside macro definitions when necessary.) + Functions not to write ---------------------- @@ -217,7 +326,25 @@ end-users that they aren't expected to understand the message (perhaps with a string like "internal error"). Option (A) is to be preferred to option (B). +Assertions In Tor +----------------- + +Assertions should be used for bug-detection only. Don't use assertions to +detect bad user inputs, network errors, resource exhaustion, or similar +issues. + +Tor is always built with assertions enabled, so try to only use +`tor_assert()` for cases where you are absolutely sure that crashing is the +least bad option. Many bugs have been caused by use of `tor_assert()` when +another kind of check would have been safer. + +If you're writing an assertion to test for a bug that you _can_ recover from, +use `tor_assert_nonfatal()` in place of `tor_assert()`. If you'd like to +write a conditional that incorporates a nonfatal assertion, use the `BUG()` +macro, as in: + if (BUG(ptr == NULL)) + return -1; Doxygen comment conventions --------------------------- diff --git a/doc/HACKING/CodingStandardsRust.md b/doc/HACKING/CodingStandardsRust.md new file mode 100644 index 0000000000..d0b17c1604 --- /dev/null +++ b/doc/HACKING/CodingStandardsRust.md @@ -0,0 +1,469 @@ + + 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 diff --git a/doc/HACKING/GettingStarted.md b/doc/HACKING/GettingStarted.md index 0295adc1ff..0c42404634 100644 --- a/doc/HACKING/GettingStarted.md +++ b/doc/HACKING/GettingStarted.md @@ -11,8 +11,9 @@ whole Tor ecosystem.) If you are looking for a more bare-bones, less user-friendly information -dump of important information, you might like reading doc/HACKING -instead. You should probably read it before you write your first patch. +dump of important information, you might like reading the "torguts" +documents linked to below. You should probably read it before you write +your first patch. Required background diff --git a/doc/HACKING/GettingStartedRust.md b/doc/HACKING/GettingStartedRust.md new file mode 100644 index 0000000000..a5253b46a6 --- /dev/null +++ b/doc/HACKING/GettingStartedRust.md @@ -0,0 +1,161 @@ + + Hacking on Rust in Tor +======================== + + Getting Started +----------------- + +Please read or review our documentation on Rust coding standards +(`.../doc/HACKING/CodingStandardsRust.md`) before doing anything. + +Please also read +[the Rust Code of Conduct](https://www.rust-lang.org/en-US/conduct.html). We aim +to follow the good example set by the Rust community and be excellent to one +another. Let's be careful with each other, so we can be memory-safe together! + +Next, please contact us before rewriting anything! Rust in Tor is still an +experiment. It is an experiment that we very much want to see succeed, so we're +going slowly and carefully. For the moment, it's also a completely +volunteer-driven effort: while many, if not most, of us are paid to work on Tor, +we are not yet funded to write Rust code for Tor. Please be patient with the +other people who are working on getting more Rust code into Tor, because they +are graciously donating their free time to contribute to this effort. + + Resources for learning Rust +----------------------------- + +**Beginning resources** + +The primary resource for learning Rust is +[The Book](https://doc.rust-lang.org/book/). If you'd like to start writing +Rust immediately, without waiting for anything to install, there is +[an interactive browser-based playground](https://play.rust-lang.org/). + +**Advanced resources** + +If you're interested in playing with various Rust compilers and viewing a very +nicely displayed output of the generated assembly, there is +[the Godbolt compiler explorer](https://rust.godbolt.org/) + +For learning how to write unsafe Rust, read +[The Rustonomicon](https://doc.rust-lang.org/nomicon/). + +For learning everything you ever wanted to know about Rust macros, there is +[The Little Book of Rust Macros](https://danielkeep.github.io/tlborm/book/index.html). + +For learning more about FFI and Rust, see Jake Goulding's +[Rust FFI Omnibus](http://jakegoulding.com/rust-ffi-omnibus/). + + Compiling Tor with Rust enabled +--------------------------------- + +You will need to run the `configure` script with the `--enable-rust` flag to +explicitly build with Rust. Additionally, you will need to specify where to +fetch Rust dependencies, as we allow for either fetching dependencies from Cargo +or specifying a local directory. + +**Fetch dependencies from Cargo** + + ./configure --enable-rust --enable-cargo-online-mode + +**Using a local dependency cache** + +**NOTE**: local dependency caches which were not *originally* created via + `--enable-cargo-online-mode` are broken. See https://bugs.torproject.org/22907 + +To specify a local directory: + + RUST_DEPENDENCIES='path_to_dependencies_directory' ./configure --enable-rust + +(Note that RUST_DEPENDENCIES must be the full path to the directory; it cannot +be relative.) + +You'll need the following Rust dependencies (as of this writing): + + libc==0.2.22 + +To get them, do: + + mkdir path_to_dependencies_directory + cd path_to_dependencies_directory + git clone https://github.com/rust-lang/libc + cd libc + git checkout 0.2.22 + cargo package + cd .. + ln -s libc/target/package/libc-0.2.22 libc-0.2.22 + + + Identifying which modules to rewrite +====================================== + +The places in the Tor codebase that are good candidates for porting to Rust are: + +1. loosely coupled to other Tor submodules, +2. have high test coverage, and +3. would benefit from being implemented in a memory safe language. + +Help in either identifying places such as this, or working to improve existing +areas of the C codebase by adding regression tests and simplifying dependencies, +would be really helpful. + +Furthermore, as submodules in C are implemented in Rust, this is a good +opportunity to refactor, add more tests, and split modules into smaller areas of +responsibility. + +A good first step is to build a module-level callgraph to understand how +interconnected your target module is. + + git clone https://git.torproject.org/user/nickm/calltool.git + cd tor + CFLAGS=0 ./configure + ../calltool/src/main.py module_callgraph + +The output will tell you each module name, along with a set of every module that +the module calls. Modules which call fewer other modules are better targets. + + Writing your Rust module +========================== + +Strive to change the C API as little as possible. + +We are currently targetting Rust nightly, *for now*. We expect this to change +moving forward, as we understand more about which nightly features we need. It +is on our TODO list to try to cultivate good standing with various distro +maintainers of `rustc` and `cargo`, in order to ensure that whatever version we +solidify on is readily available. + + Adding your Rust module to Tor's build system +----------------------------------------------- + +0. Your translation of the C module should live in its own crate(s) + in the `.../tor/src/rust/` directory. +1. Add your crate to `.../tor/src/rust/Cargo.toml`, in the + `[workspace.members]` section. +2. Append your crate's static library to the `rust_ldadd` definition + (underneath `if USE_RUST`) in `.../tor/Makefile.am`. + + How to test your Rust code +---------------------------- + +Everything should be tested full stop. Even non-public functionality. + +Be sure to edit `.../tor/src/test/test_rust.sh` to add the name of your crate to +the `crates` variable! This will ensure that `cargo test` is run on your crate. + +Configure Tor's build system to build with Rust enabled: + + ./configure --enable-fatal-warnings --enable-rust --enable-cargo-online-mode + +Tor's test should be run by doing: + + make check + +Tor's integration tests should also pass: + + make test-stem + + Submitting a patch +===================== + +Please follow the instructions in `.../doc/HACKING/GettingStarted.md`. diff --git a/doc/HACKING/HelpfulTools.md b/doc/HACKING/HelpfulTools.md index b8ba2aa408..f919d08ec1 100644 --- a/doc/HACKING/HelpfulTools.md +++ b/doc/HACKING/HelpfulTools.md @@ -111,15 +111,19 @@ Running gcov for unit test coverage (On OSX, you'll need to start with `--enable-coverage CC=clang`.) -Then, look at the .gcov files in `coverage-output`. '-' before a line means -that the compiler generated no code for that line. '######' means that the -line was never reached. Lines with numbers were called that number of times. - If that doesn't work: * Try configuring Tor with `--disable-gcc-hardening` * You might need to run `make clean` after you run `./configure`. +Then, look at the .gcov files in `coverage-output`. '-' before a line means +that the compiler generated no code for that line. '######' means that the +line was never reached. Lines with numbers were called that number of times. + +For more details about how to read gcov output, see the [Invoking +gcov](https://gcc.gnu.org/onlinedocs/gcc/Invoking-Gcov.html) chapter +of the GCC manual. + If you make changes to Tor and want to get another set of coverage results, you can run `make reset-gcov` to clear the intermediary gcov output. @@ -128,9 +132,13 @@ a meaningful diff between them, you can run: ./scripts/test/cov-diff coverage-output1 coverage-output2 | less -In this diff, any lines that were visited at least once will have coverage -"1". This lets you inspect what you (probably) really want to know: which -untested lines were changed? Are there any new untested lines? +In this diff, any lines that were visited at least once will have coverage "1", +and line numbers are deleted. This lets you inspect what you (probably) really +want to know: which untested lines were changed? Are there any new untested +lines? + +If you run ./scripts/test/cov-exclude, it marks excluded unreached +lines with 'x', and excluded reached lines with '!!!'. Running integration tests ------------------------- diff --git a/doc/HACKING/HowToReview.md b/doc/HACKING/HowToReview.md index d53318942f..2d1f3d1c9e 100644 --- a/doc/HACKING/HowToReview.md +++ b/doc/HACKING/HowToReview.md @@ -19,6 +19,8 @@ Top-level smell-checks - Does `make check-spaces` pass? +- Does `make check-changes` pass? + - Does it have a reasonable amount of tests? Do they pass? Do they leak memory? @@ -32,6 +34,7 @@ Top-level smell-checks - If this changes Tor's behavior on the wire, is there a design proposal? +- If this changes anything in the code, is there a "changes" file? Let's look at the code! diff --git a/doc/HACKING/ReleasingTor.md b/doc/HACKING/ReleasingTor.md index 4ece4d7a1d..62029b44f0 100644 --- a/doc/HACKING/ReleasingTor.md +++ b/doc/HACKING/ReleasingTor.md @@ -8,7 +8,11 @@ new Tor release: === 0. Preliminaries 1. Get at least three of weasel/arma/Sebastian/Sina to put the new - version number in their approved versions list. + version number in their approved versions list. Give them a few + days to do this if you can. + +2. If this is going to be an important security release, give the packagers + some advance warning: See this list of packagers in IV.3 below. === I. Make sure it works @@ -26,19 +30,24 @@ new Tor release: What about Coverity Scan? + What about clan scan-build? + Does 'make distcheck' complain? - How about 'make test-stem' and 'make test-network'? + How about 'make test-stem' and 'make test-network' and + `make test-network-full`? - Are all those tests still happy with --enable-expensive-hardening ? Any memory leaks? -=== II. Write a changelog. +=== II. Write a changelog -1. Gather the `changes/*` files into a changelog entry, rewriting many +1a. (Alpha release variant) + + Gather the `changes/*` files into a changelog entry, rewriting many of them and reordering to focus on what users and funders would find interesting and understandable. @@ -83,6 +92,15 @@ new Tor release: 4. Run `./scripts/maint/format_changelog.py --inplace` to make it prettier +1b. (old-stable release variant) + + For stable releases that backport things from later, we try to compose + their releases, we try to make sure that we keep the changelog entries + identical to their original versions, with a 'backport from 0.x.y.z' + note added to each section. So in this case, once you have the items + from the changes files copied together, don't use them to build a new + changelog: instead, look up the corrected versions that were merged + into ChangeLog in the master branch, and use those. 2. Compose a short release blurb to highlight the user-facing changes. Insert said release blurb into the ChangeLog stanza. If it's @@ -109,7 +127,7 @@ new Tor release: === III. Making the source release. 1. In `maint-0.?.x`, bump the version number in `configure.ac` and run - `scripts/maint/updateVersions.pl` to update version numbers in other + `perl scripts/maint/updateVersions.pl` to update version numbers in other places, and commit. Then merge `maint-0.?.x` into `release-0.?.x`. (NOTE: To bump the version number, edit `configure.ac`, and then run @@ -147,10 +165,13 @@ new Tor release: - {weasel,gk,mikeperry} at torproject dot org - {blueness} at gentoo dot org - {paul} at invizbox dot io + - {vincent} at invizbox dot com - {lfleischer} at archlinux dot org - {Nathan} at freitas dot net - {mike} at tig dot as - {tails-rm} at boum dot org + - {simon} at sdeziel.info + - {yuri} at rawbw.com 4. Add the version number to Trac. To do this, go to Trac, log in, select "Admin" near the top of the screen, then select "Versions" from @@ -162,7 +183,7 @@ new Tor release: 5. Mail the release blurb and ChangeLog to tor-talk (development release) or tor-announce (stable). - Post the changelog on the the blog as well. You can generate a + Post the changelog on the blog as well. You can generate a blog-formatted version of the changelog with the -B option to format-changelog. diff --git a/doc/HACKING/WritingTests.md b/doc/HACKING/WritingTests.md index 4dae41e922..cc393494ec 100644 --- a/doc/HACKING/WritingTests.md +++ b/doc/HACKING/WritingTests.md @@ -91,6 +91,9 @@ coverage percentage. For a summary of the test coverage for each _function_, run `./scripts/test/cov-display -f ${TMPDIR}/*`. +For more details on using gcov, including the helper scripts in +scripts/test, see HelpfulTools.md. + ### Comparing test coverage Sometimes it's useful to compare test coverage for a branch you're writing to @@ -117,7 +120,8 @@ with LCOV_EXCL_START... LCOV_EXCL_STOP. Note that older versions of lcov don't understand these lines. You can post-process .gcov files to make these lines 'unreached' by -running ./scripts/test/cov-exclude on them. +running ./scripts/test/cov-exclude on them. It marks excluded +unreached lines with 'x', and excluded reached lines with '!!!'. Note: you should never do this unless the line is meant to 100% unreachable by actual code. |