Opened 8 months ago

Closed 6 months ago

#27272 closed defect (fixed)

ASan is incompatible with Rust's jemalloc on Travis

Reported by: alexcrichton Owned by:
Priority: High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust
Cc: Actual Points:
Parent ID: #25386 Points:
Reviewer: teor Sponsor:

Description

In helping to debug https://trac.torproject.org/projects/tor/ticket/25386 I've found that many of the segfaults at runtime are attributable to Rust pulling in jemalloc by default for tests, which apparently doesn't play well with ASan when linked in.

I've found that using code like:

#[global_allocator]
static ALLOCATOR: std::alloc::System = std::alloc::System;

is enough to solve the problem. This tells Rust that it should use the system allocator (e.g. the malloc/free symbols) instead of jemalloc. This was stabilized very recently in Rust, though, so using it may not be so trivial!

In some local testing I was able to get away with adding the above declaration to the tor_allocate crate for the most part, but crates like crypto, external, and smartlist don't already link to tor_allocate and needed the above declaration with a #[cfg(test)] as well. Once this was all added though I mostly no longer saw segfaults related to jemalloc and ASan

Child Tickets

Change History (10)

comment:1 Changed 8 months ago by alexcrichton

As a follow-up to this as well, I've found that one final reason (I hope!) as to why Travis is failing with hardened code and Rust is due to the usage of link_rust.sh in RUSTFLAGS. By default Tor is using cargo test without a --target argument which means that RUSTFLAGS is *also* passed to compilation of build scripts. These build scripts don't actually link to C code and don't need the -fsanitize=address treatment, but the build scripts (often in upstream crates) are compiled with Jemalloc as the crate's allocator. By being linked with link_rust.sh this causes them to segfault at runtime, presumably because ASan is not compatible with jemalloc.

One way to fix this I know if (and yes, I know this is a weird fix!) is to pass --target x86_64-unknown-linux-gnu. Basically you pass --target to Cargo as-if you're cross-compiling, except you're not actually given the actual targets in play. This causes Cargo to not pass RUSTFLAGS to the build script compilations, meaning that build scripts are not compiled with sanitizers.

This part is definitely the trickiest, and I haven't quite figured out yet how to integrate this into the build system :(

comment:2 Changed 8 months ago by nickm

Component: - Select a componentCore Tor/Tor
Keywords: rust added
Milestone: Tor: 0.3.5.x-final

comment:3 Changed 8 months ago by teor

Thanks for working on these issues!

Can we compile every crate with -fsanitize=address in every mode?
It might be easier to be consistent, than have some objects compiled with some settings, and other objects compiled with other settings.

comment:4 Changed 8 months ago by alexcrichton

Ah unfortunately I haven't dug into why exactly jemalloc and ASan are incompatible, I just assumed it was some fundamental thing. I think that while it's in theory possible to compile crates with -fsanitize=address it wouldn't affect precompiled crates like the standard library (which includes jemalloc).

In that sense I don't think it'd help here (specifically w/ the jemalloc problem). AFAIK the only solution is to get things to stop using jemalloc entirely. It may be worth investigating though why jemalloc is segfaulting with ASan, and it may or may not be a patch we could apply upstream.

FWIW we'd like to move away from jemalloc as the default allocator, we're just having a tough time doing so unfortunately.

comment:5 Changed 8 months ago by teor

Parent ID: #25386

comment:6 Changed 7 months ago by nickm

Priority: MediumHigh

comment:7 Changed 7 months ago by alexcrichton

I've sent a PR which I believe fixes this at https://github.com/torproject/tor/pull/381

comment:8 Changed 7 months ago by nickm

Status: newneeds_review

comment:9 Changed 6 months ago by dgoulet

Reviewer: teor

comment:10 Changed 6 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I've merged the PR into 0.3.5! See #27273 for more commentary.

Note: See TracTickets for help on using tickets.