From 3dca87e1f98adcdffb962de381d1e5d5410f3d54 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Oct 2018 22:48:46 -0700 Subject: [PATCH 1/7] Reenable hardening options with Rust on Travis Previously the sanitizers are forcibly disabled as they were found to be incompatible with Rust code. The nightly channel of Rust, however, now has some fixes which should make this disabling no longer necessary. --- .travis.yml | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/.travis.yml b/.travis.yml index 34b7595eb3..54b87975e9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -33,12 +33,8 @@ env: ## We don't list default variable values, because we set the defaults ## in global (or the default is unset) - - ## We turn off hardening for Rust builds, because they are incompatible, - ## and it's going to take a while for them to be fixed. See: - ## https:/trac.torproject.org/projects/tor/ticket/25386 - ## https:/trac.torproject.org/projects/tor/ticket/26398 ## TOR_RUST_DEPENDENCIES is spelt RUST_DEPENDENCIES in 0.3.2 - - RUST_OPTIONS="--enable-rust" TOR_RUST_DEPENDENCIES=true HARDENING_OPTIONS="" + - RUST_OPTIONS="--enable-rust" TOR_RUST_DEPENDENCIES=true matrix: ## include creates builds with gcc, linux, sudo: false @@ -52,10 +48,9 @@ matrix: ## We check asciidoc with distcheck, to make sure we remove doc products - env: DISTCHECK="yes" ASCIIDOC_OPTIONS="" ## Check rust online with distcheck, to make sure we remove rust products - ## But without hardening (see above) - - env: DISTCHECK="yes" RUST_OPTIONS="--enable-rust --enable-cargo-online-mode" HARDENING_OPTIONS="" + - env: DISTCHECK="yes" RUST_OPTIONS="--enable-rust --enable-cargo-online-mode" ## Check disable module dirauth with and without rust - - env: MODULES_OPTIONS="--disable-module-dirauth" RUST_OPTIONS="--enable-rust" TOR_RUST_DEPENDENCIES=true HARDENING_OPTIONS="" + - env: MODULES_OPTIONS="--disable-module-dirauth" RUST_OPTIONS="--enable-rust" TOR_RUST_DEPENDENCIES=true - env: MODULES_OPTIONS="--disable-module-dirauth" ## Uncomment to allow the build to report success (with non-required @@ -167,8 +162,8 @@ install: - if [[ "$ASCIIDOC_OPTIONS" == "" ]] && [[ "$TRAVIS_OS_NAME" == "osx" ]]; then export XML_CATALOG_FILES="/usr/local/etc/xml/catalog"; fi ## If we're using Rust, download rustup - if [[ "$RUST_OPTIONS" != "" ]]; then curl -Ssf -o rustup.sh https://sh.rustup.rs; fi - ## Install the stable channels of rustc and cargo and setup our toolchain environment - - if [[ "$RUST_OPTIONS" != "" ]]; then sh rustup.sh -y --default-toolchain stable; fi + ## Install the nightly channels of rustc and cargo and setup our toolchain environment + - if [[ "$RUST_OPTIONS" != "" ]]; then sh rustup.sh -y --default-toolchain nightly; fi - if [[ "$RUST_OPTIONS" != "" ]]; then source $HOME/.cargo/env; fi ## If we're testing rust builds in offline-mode, then set up our vendored dependencies - if [[ "$TOR_RUST_DEPENDENCIES" == "true" ]]; then export TOR_RUST_DEPENDENCIES=$PWD/src/ext/rust/crates; fi From 6ebb2c46d5eae7bae8d827fdc68d3ed58b16e95a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Oct 2018 22:50:08 -0700 Subject: [PATCH 2/7] Remove the `link_rust.sh.in` script This is no longer necessary with upstream rust-lang/rust changes as well as some local tweaks. Namely: * The `-fsanitize=address`-style options are now passed via `-C link-args` through `RUSTFLAGS`. This obviates the need for the shell script. * The `-C default-linker-libraries`, disabling `-nodefaultlibs`, is passed through `RUSTFLAGS`, which is necessary to ensure that `-fsanitize=address` links correctly. * The `-C linker` option is passed to ensure we're using the same C compiler as normal C code, although it has a bit of hackery to only get the `gcc` out of `gcc -std=c99` --- configure.ac | 9 ++++----- link_rust.sh.in | 10 ---------- src/test/include.am | 3 +-- 3 files changed, 5 insertions(+), 17 deletions(-) delete mode 100644 link_rust.sh.in diff --git a/configure.ac b/configure.ac index dbeba512ca..ef8bb4e351 100644 --- a/configure.ac +++ b/configure.ac @@ -1210,17 +1210,17 @@ dnl variable. RUST_LINKER_OPTIONS="" if test "x$have_clang" = "xyes"; then if test "x$CFLAGS_ASAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS $CFLAGS_ASAN" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=$CFLAGS_ASAN" fi if test "x$CFLAGS_UBSAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS $CFLAGS_UBSAN" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=$CFLAGS_UBSAN" fi else if test "x$CFLAGS_ASAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -lasan" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=-fsanitize=address" fi if test "x$CFLAGS_UBSAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -lubsan" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=-fsanitize=undefined" fi fi AC_SUBST(RUST_LINKER_OPTIONS) @@ -2416,7 +2416,6 @@ AC_CONFIG_FILES([ Doxyfile Makefile config.rust - link_rust.sh contrib/dist/suse/tor.sh contrib/operator-tools/tor.logrotate contrib/dist/tor.sh diff --git a/link_rust.sh.in b/link_rust.sh.in deleted file mode 100644 index 59f4142baa..0000000000 --- a/link_rust.sh.in +++ /dev/null @@ -1,10 +0,0 @@ -#!/bin/sh -# -# A linker script used when building Rust tests. Autoconf makes link_rust.sh -# from link_rust_sh.in, and uses it to pass extra options to the linker -# when linking Rust stuff. -# -# We'd like to remove the need for this, but build.rs doesn't let us pass -# -static-libasan and -static-libubsan to the linker. - -$CCLD @RUST_LINKER_OPTIONS@ "$@" diff --git a/src/test/include.am b/src/test/include.am index 1055cd0a81..4314581082 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -12,8 +12,7 @@ TESTS_ENVIRONMENT = \ export EXTRA_CARGO_OPTIONS="$(EXTRA_CARGO_OPTIONS)"; \ export CARGO_ONLINE="$(CARGO_ONLINE)"; \ export CCLD="$(CCLD)"; \ - chmod +x "$(abs_top_builddir)/link_rust.sh"; \ - export RUSTFLAGS="-C linker=$(abs_top_builddir)/link_rust.sh"; + export RUSTFLAGS="-C linker=`echo '$(CC)' | cut -d' ' -f 1` $(RUST_LINKER_OPTIONS) -C default-linker-libraries"; TESTSCRIPTS = \ src/test/fuzz_static_testcases.sh \ From 74c1e44746a7d9c810f095177af88b7dbaafd3e3 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Oct 2018 22:54:20 -0700 Subject: [PATCH 3/7] Fix segfaults related to sanitizers+jemalloc It looks to be the case that Rust's standard allocator, jemalloc, is incompatible with sanitizers. The incompatibility, for whatever reason, seems to cause segfaults at runtime when jemalloc is linked with sanitizers. Without actually trying to figure out what's going on here this commit instead takes the hammer of "let's remove jemalloc when testing". The `tor_allocate` crate now by default switches to the system allocator (eventually this will want to be the tor allocator). Most crates then link to `tor_allocate` ot pick this up, but the `smartlist` crate had to manually switch to the system allocator in testing and the `external` crate had to be sure to link to `tor_allocate`. The final gotcha here is that this patch also switches to unconditionally passing `--target` to Cargo. For weird and arcane reasons passing `--target` with the host target of the compiler (which Cargo otherwise uses as the default) is different than not passing `--target` at all. This ensure that our custom `RUSTFLAGS` with sanitizer options doesn't make its way into build scripts, just the final testing artifacts. --- src/rust/Cargo.lock | 1 + src/rust/external/Cargo.toml | 5 ++--- src/rust/external/lib.rs | 2 +- src/rust/smartlist/lib.rs | 9 +++++++++ src/rust/tor_allocate/lib.rs | 5 +++++ src/test/test_rust.sh | 10 +++++++++- 6 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/rust/Cargo.lock b/src/rust/Cargo.lock index 1d2a7359aa..7d6a6635c5 100644 --- a/src/rust/Cargo.lock +++ b/src/rust/Cargo.lock @@ -26,6 +26,7 @@ version = "0.0.1" dependencies = [ "libc 0.2.39 (registry+https://github.com/rust-lang/crates.io-index)", "smartlist 0.0.1", + "tor_allocate 0.0.1", ] [[package]] diff --git a/src/rust/external/Cargo.toml b/src/rust/external/Cargo.toml index 4735144ee6..d5c3a739e0 100644 --- a/src/rust/external/Cargo.toml +++ b/src/rust/external/Cargo.toml @@ -5,9 +5,8 @@ name = "external" [dependencies] libc = "=0.2.39" - -[dependencies.smartlist] -path = "../smartlist" +smartlist = { path = "../smartlist" } +tor_allocate = { path = "../tor_allocate" } [lib] name = "external" diff --git a/src/rust/external/lib.rs b/src/rust/external/lib.rs index b72a4f6e4c..d68036fcad 100644 --- a/src/rust/external/lib.rs +++ b/src/rust/external/lib.rs @@ -8,7 +8,7 @@ //! module implementing this functionality repeatedly. extern crate libc; - +extern crate tor_allocate; extern crate smartlist; pub mod crypto_digest; diff --git a/src/rust/smartlist/lib.rs b/src/rust/smartlist/lib.rs index 2716842af2..34d0b907ed 100644 --- a/src/rust/smartlist/lib.rs +++ b/src/rust/smartlist/lib.rs @@ -6,3 +6,12 @@ extern crate libc; mod smartlist; pub use smartlist::*; + +// When testing we may be compiled with sanitizers which are incompatible with +// Rust's default allocator, jemalloc (unsure why at this time). Most crates +// link to `tor_allocate` which switches by default to a non-jemalloc allocator, +// but we don't already depend on `tor_allocate` so make sure that while testing +// we don't use jemalloc. (but rather malloc/free) +#[global_allocator] +#[cfg(test)] +static A: std::alloc::System = std::alloc::System; diff --git a/src/rust/tor_allocate/lib.rs b/src/rust/tor_allocate/lib.rs index 5a355bc8d6..1cfa0b5178 100644 --- a/src/rust/tor_allocate/lib.rs +++ b/src/rust/tor_allocate/lib.rs @@ -11,5 +11,10 @@ extern crate libc; +use std::alloc::System; + mod tor_allocate; pub use tor_allocate::*; + +#[global_allocator] +static A: System = System; diff --git a/src/test/test_rust.sh b/src/test/test_rust.sh index a1a56af480..00b3e88d37 100755 --- a/src/test/test_rust.sh +++ b/src/test/test_rust.sh @@ -5,12 +5,20 @@ set -e export LSAN_OPTIONS=suppressions=${abs_top_srcdir:-../../..}/src/test/rust_supp.txt +# When testing Cargo we pass a number of very specific linker flags down +# through Cargo. We do not, however, want these flags to affect things like +# build scripts, only the tests that we're compiling. To ensure this happens +# we unconditionally pass `--target` into Cargo, ensuring that `RUSTFLAGS` in +# the environment won't make their way into build scripts. +rustc_host=$(rustc -vV | grep host | sed 's/host: //') + for cargo_toml_dir in "${abs_top_srcdir:-../../..}"/src/rust/*; do if [ -e "${cargo_toml_dir}/Cargo.toml" ]; then cd "${abs_top_builddir:-../../..}/src/rust" && \ CARGO_TARGET_DIR="${abs_top_builddir:-../../..}/src/rust/target" \ "${CARGO:-cargo}" test ${CARGO_ONLINE-"--frozen"} \ - --features "test_linking_hack" \ + --features "test_linking_hack" \ + --target $rustc_host \ ${EXTRA_CARGO_OPTIONS} \ --manifest-path "${cargo_toml_dir}/Cargo.toml" || exitcode=1 fi From 757a2360a41a5de2abd283ddee9fd26bb045bc04 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Oct 2018 22:57:38 -0700 Subject: [PATCH 4/7] Remove `[features]` from workspace Cargo.toml Unfortunately Cargo doesn't actually parse these! Cargo should probably print a warning saying they're not used... --- src/rust/Cargo.toml | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/rust/Cargo.toml b/src/rust/Cargo.toml index e399dbb33a..83f9629660 100644 --- a/src/rust/Cargo.toml +++ b/src/rust/Cargo.toml @@ -13,19 +13,3 @@ members = [ [profile.release] debug = true panic = "abort" - -[features] -default = [] -# If this feature is enabled, test code which calls Tor C code from Rust will -# execute with `cargo test`. Due to numerous linker issues (#25386), this is -# currently disabled by default. Crates listed here are those which, in their -# unittests, doctests, and/or integration tests, call C code. -test-c-from-rust = [ - "crypto/test-c-from-rust", -] - -# We have to define a feature here because doctests don't get cfg(test), -# and we need to disable some C dependencies when running the doctests -# because of the various linker issues. See -# https://github.com/rust-lang/rust/issues/45599 -test_linking_hack = [] From 38d644c94bf1a42fcec40843e817c460e9e752de Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 1 Oct 2018 22:58:44 -0700 Subject: [PATCH 5/7] Remove rlib+staticlib configuration for Rust crates Only the final crate needs to be a `staticlib`, no need for all the intermediate steps to produce staticlibs! --- src/rust/crypto/Cargo.toml | 1 - src/rust/external/Cargo.toml | 1 - src/rust/protover/Cargo.toml | 1 - src/rust/smartlist/Cargo.toml | 1 - src/rust/tor_allocate/Cargo.toml | 1 - src/rust/tor_log/Cargo.toml | 1 - src/rust/tor_rust/Cargo.toml | 2 +- src/rust/tor_util/Cargo.toml | 1 - 8 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/rust/crypto/Cargo.toml b/src/rust/crypto/Cargo.toml index 6ebfe0dc11..a7ff7f78d9 100644 --- a/src/rust/crypto/Cargo.toml +++ b/src/rust/crypto/Cargo.toml @@ -9,7 +9,6 @@ build = "../build.rs" [lib] name = "crypto" path = "lib.rs" -crate_type = ["rlib", "staticlib"] [dependencies] libc = "=0.2.39" diff --git a/src/rust/external/Cargo.toml b/src/rust/external/Cargo.toml index d5c3a739e0..5f443645bb 100644 --- a/src/rust/external/Cargo.toml +++ b/src/rust/external/Cargo.toml @@ -11,7 +11,6 @@ tor_allocate = { path = "../tor_allocate" } [lib] name = "external" path = "lib.rs" -crate_type = ["rlib", "staticlib"] [features] # We have to define a feature here because doctests don't get cfg(test), diff --git a/src/rust/protover/Cargo.toml b/src/rust/protover/Cargo.toml index 2f7783e76c..84a7c71c1a 100644 --- a/src/rust/protover/Cargo.toml +++ b/src/rust/protover/Cargo.toml @@ -31,4 +31,3 @@ path = "../tor_log" [lib] name = "protover" path = "lib.rs" -crate_type = ["rlib", "staticlib"] diff --git a/src/rust/smartlist/Cargo.toml b/src/rust/smartlist/Cargo.toml index 4ecdf50869..a5afe7bf74 100644 --- a/src/rust/smartlist/Cargo.toml +++ b/src/rust/smartlist/Cargo.toml @@ -9,7 +9,6 @@ libc = "0.2.39" [lib] name = "smartlist" path = "lib.rs" -crate_type = ["rlib", "staticlib"] [features] # We have to define a feature here because doctests don't get cfg(test), diff --git a/src/rust/tor_allocate/Cargo.toml b/src/rust/tor_allocate/Cargo.toml index 7bb3b9887f..06ac605f17 100644 --- a/src/rust/tor_allocate/Cargo.toml +++ b/src/rust/tor_allocate/Cargo.toml @@ -9,7 +9,6 @@ libc = "=0.2.39" [lib] name = "tor_allocate" path = "lib.rs" -crate_type = ["rlib", "staticlib"] [features] # We have to define a feature here because doctests don't get cfg(test), diff --git a/src/rust/tor_log/Cargo.toml b/src/rust/tor_log/Cargo.toml index 1aa9be0612..14d9ae803a 100644 --- a/src/rust/tor_log/Cargo.toml +++ b/src/rust/tor_log/Cargo.toml @@ -6,7 +6,6 @@ authors = ["The Tor Project"] [lib] name = "tor_log" path = "lib.rs" -crate_type = ["rlib", "staticlib"] [features] # We have to define a feature here because doctests don't get cfg(test), diff --git a/src/rust/tor_rust/Cargo.toml b/src/rust/tor_rust/Cargo.toml index 1523ee0dd1..35c629882e 100644 --- a/src/rust/tor_rust/Cargo.toml +++ b/src/rust/tor_rust/Cargo.toml @@ -6,7 +6,7 @@ version = "0.1.0" [lib] name = "tor_rust" path = "lib.rs" -crate_type = ["rlib", "staticlib"] +crate_type = ["staticlib"] [dependencies.tor_util] path = "../tor_util" diff --git a/src/rust/tor_util/Cargo.toml b/src/rust/tor_util/Cargo.toml index 51e4bd9c5d..9ffaeda8a6 100644 --- a/src/rust/tor_util/Cargo.toml +++ b/src/rust/tor_util/Cargo.toml @@ -6,7 +6,6 @@ version = "0.0.1" [lib] name = "tor_util" path = "lib.rs" -crate_type = ["rlib", "staticlib"] [dependencies.tor_allocate] path = "../tor_allocate" From 82857849662189d25e1a06bf6b764c64d2468168 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 29 Oct 2018 10:00:23 -0700 Subject: [PATCH 6/7] Only pass `-C default-linker-libraries` with sanitizers This'll help retain test compatibility until 1.31.0 is released! --- configure.ac | 8 ++++---- src/test/include.am | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/configure.ac b/configure.ac index ef8bb4e351..8f8aa438df 100644 --- a/configure.ac +++ b/configure.ac @@ -1210,17 +1210,17 @@ dnl variable. RUST_LINKER_OPTIONS="" if test "x$have_clang" = "xyes"; then if test "x$CFLAGS_ASAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=$CFLAGS_ASAN" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=$CFLAGS_ASAN -Cdefault-linker-libraries" fi if test "x$CFLAGS_UBSAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=$CFLAGS_UBSAN" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=$CFLAGS_UBSAN -Cdefault-linker-libraries" fi else if test "x$CFLAGS_ASAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=-fsanitize=address" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=-fsanitize=address -Cdefault-linker-libraries" fi if test "x$CFLAGS_UBSAN" != "x"; then - RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=-fsanitize=undefined" + RUST_LINKER_OPTIONS="$RUST_LINKER_OPTIONS -Clink-arg=-fsanitize=undefined -Cdefault-linker-libraries" fi fi AC_SUBST(RUST_LINKER_OPTIONS) diff --git a/src/test/include.am b/src/test/include.am index 4314581082..ecb7689579 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -12,7 +12,7 @@ TESTS_ENVIRONMENT = \ export EXTRA_CARGO_OPTIONS="$(EXTRA_CARGO_OPTIONS)"; \ export CARGO_ONLINE="$(CARGO_ONLINE)"; \ export CCLD="$(CCLD)"; \ - export RUSTFLAGS="-C linker=`echo '$(CC)' | cut -d' ' -f 1` $(RUST_LINKER_OPTIONS) -C default-linker-libraries"; + export RUSTFLAGS="-C linker=`echo '$(CC)' | cut -d' ' -f 1` $(RUST_LINKER_OPTIONS)"; TESTSCRIPTS = \ src/test/fuzz_static_testcases.sh \ From ee1cc0feaef6a13f3edf47a42b0c8e5a07603f9c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 30 Oct 2018 08:45:35 -0400 Subject: [PATCH 7/7] Add a changes file for Alex Crichton's rust fixes. --- changes/rust_asan | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changes/rust_asan diff --git a/changes/rust_asan b/changes/rust_asan new file mode 100644 index 0000000000..1ca7ae6888 --- /dev/null +++ b/changes/rust_asan @@ -0,0 +1,8 @@ + o Major bugfixes (compilation, rust): + - Rust tests can now build and run successfully with the + --enable-fragile-hardening option enabled. + Doing this currently requires the rust beta channel; it will + be possible with stable rust as of rust version 1.31 is out. + Patch from Alex Crichton. + Fixes bugs 27272, 27273, and 27274. + Bugfix on 0.3.1.1-alpha.