ASan fails to link on Travis due to rustc and linker arguments

In helping to debug https://trac.torproject.org/projects/tor/ticket/25386 I've found that a number of the link errors are attributable to the linker invocation on Travis. The recent link_rust.sh script is definitely necessary I think, except that I believe it needs a few tweaks:

• The -lasan argument should be replaced with -fsanitizer=address I believe to ensure that the C compiler links all of its relevant libraries (as they may have different names and different numbers of libs on some platforms I think).
• Second, the Rust compiler by default passes -nodefaultlibs to the linker which means that the linker script also passes this along. It looks, however, like this is incompatible with -fsanitizer=address, causing link errors. This argument should be safe to strip out though.

The first bullet was easy enough to fix locally and the second bullet was fixable by changing link_rust.sh to a bash script and then using something like:

linker_args="$@"                                           
$CCLD @RUST_LINKER_OPTIONS@ ${linker_args//-nodefaultlibs/}

Trac:
Username: alexcrichton

changed milestone to %Tor: 0.3.5.x-final

added 035-can component::core tor/tor milestone::Tor: 0.3.5.x-final parent::25386 priority::high reporter::alexcrichton resolution::wontfix reviewer::teor rust severity::normal status::closed type::defect labels

Trac:
Milestone: N/A to Tor: 0.3.5.x-final
Keywords: N/A deleted, rust added
Component: - Select a component to Core Tor/Tor

Some systems don't have bash installed. Do we require bash for other build steps? If not, we should list it as a dependency.

Also, some distros install bash in /usr/local/bin, so we should use env to find it.

Is there an argument that's the opposite of -nodefaultlibs? If so, we could add it to the command-line using any shell. There doesn't seem to be a -defaultlibs in gcc, although there may be some command-line code that gives every option a negated form. https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html

Ah yeah I tried to pass -defaultlibs in the hopes that it would work, but unfortunately I also was unable to get it working. We also unfortunately don't have a way to disable this behavior in rustc (passing -nodefaultlibs), but we probably should...

Trac:
Username: alexcrichton

Trac:
Parent: N/A to #25386 (moved)

Replying to alexcrichton:

Ah yeah I tried to pass -defaultlibs in the hopes that it would work, but unfortunately I also was unable to get it working. We also unfortunately don't have a way to disable this behavior in rustc (passing -nodefaultlibs), but we probably should...

Is there a github issue for that?

Sure yeah, I've opened https://github.com/rust-lang/rust/issues/54237 for that.

Thinking about this again though, it may be the case that -nodefaultlibs is a red herring in the sense that the link errors may just be fixable by linking the libraries that otherwise wouldn't be linked. I'd have to dig more into the precise linker errors though which I sort of forget at this point :(

Trac:
Username: alexcrichton

Trac:
Priority: Medium to High

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

Trac:
Username: alexcrichton

Trac:
Status: new to needs_review

Trac:
Reviewer: N/A to teor

Okay -- Teor is swamped, so I'll try to do initial review. I've looked over this branch and re-tested it... and I think it is good to merge. I have just one question and a few TODOs before I go ahead:

The question:

• When can we switch back from "nightly" to "stable"? I don't want tor to start depending on nightly, so if there's going to be a long interval, we should add another build for rust stable (with no sanitizers).

The TODOs: (Alex, you don't need to do any of these unless you are super-psyched to participate in the minutiae of our development process)

• Add a changes file (see doc/HACKING/CodingStandards.md)
• For the " Remove the link_rust.sh.in script" commit, add a bunch of comments to include.am to explain exactly what the new RUSTFLAGS stuff does and why it's there.

Two more TODOs:

• Open a ticket for moving Travis back to rust stable
• Open a ticket for the issues identified in Alex's comment about "test_linking_hack"

This'll be able to work on stable starting with the 1.31.0 release, happening in early December. The bug fixes should be riding the trains soon to beta (happening in the next day or so). Once that happens you can switch to beta to ensure nightly features don't slip in by accident!

I'll look to see if I can tweak the commits in the coming days, but don't block on me! I probably won't be able to get to this until next Monday at the earliest

Trac:
Username: alexcrichton

Replying to alexcrichton:

This'll be able to work on stable starting with the 1.31.0 release, happening in early December. The bug fixes should be riding the trains soon to beta (happening in the next day or so). Once that happens you can switch to beta to ensure nightly features don't slip in by accident!

I think we might benefit from always building with stable, beta and nightly rust:

• If we always build Mac and Linux with nightly, we'll catch rust bugs early, before they ride the trains.
• If we always build Linux with beta and stable, then we'll catch nightly features, and also catch any bugs that slip into beta while there's still a chance to fix them. (We try to minimise our Mac builds, because they're slow.)

We currently have the following rust builds:

• Linux, rust stable, frozen rust dependencies, no hardening
• Mac, rust stable, frozen rust dependencies, no hardening
• Linux, rust stable, online rust dependencies, no hardening, distcheck
• Linux, rust stable, frozen rust dependencies, no hardening, disable module dirauth

TODO: when we merge this branch, we could switch rust versions and turn on hardening for the first three builds:

• Linux, rust nightly, frozen rust dependencies, hardening
• Mac, rust nightly, frozen rust dependencies, hardening
• Linux, rust beta, online rust dependencies, hardening, distcheck

TODO: open a ticket to switch on hardening for the rust stable build in December:

• Linux, rust stable, frozen rust dependencies, hardening, disable module dirauth

Alex/Nick, I can update the rust versions and hardening in Travis config if you'd like. Or I can review your commits.

Hm. One thing I found here in my own testing is that this code seems to fail on rust 1.29 even if the sanitizers are disabled:

  process didn't exit successfully: `rustc - --crate-name ___ --print=file-names -C linker=gcc -C default-linker-libraries --target x86_64-unknown-linux-gnu --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro` (exit code: 1)
--- stderr
error: unknown codegen option: `default-linker-libraries`

I'd either like to fix this up so that it doesn't break non-hardened builds with 1.29, or wait until 1.29 is obsolete before we merge it.

Trac:
Status: needs_review to needs_revision

Oops sorry about that, the issue of working with 1.29/1.30 should be fixed now in the latest commit I've pushed to the PR

I also don't mind rejiggering versions being tested and such, I think it definitely makes sense to test on stable and then just have a handful of nightly builders for now with hardening until that support stabilizes.

Trac:
Username: alexcrichton

Yes, this looks good!

I've added a changes file and merged it to 0.3.5 and later, since it's a clear improvement over what we have now. I've opened a new ticket #28244 (moved) for the followup items that we identified above. Thank you!

Trac:
Status: needs_revision to closed
Resolution: N/A to fixed

This branch breaks compatibility with Rust 1.26.1, which the Tor Browser team is using. See #28260 (moved).

Was this intentional? If so, we should update the changes file to document the lowest version required to build tor 0.3.5.

Trac:
Status: closed to reopened
Resolution: fixed to N/A

Hm. I'd like to restore compatibility if it isn't too much work.

On the other hand, I would also like to have a plan for keeping our required version of rust up to date, so we're not locked into 1.26 forever.

Trac:
Keywords: N/A deleted, 035-can added

Rust 1.26 is very old now. Let's give up on it.

Trac:
Resolution: N/A to wontfix
Status: reopened to closed

closed

mentioned in issue #27274 (moved)

mentioned in issue #27741 (moved)

mentioned in issue #28244 (moved)

mentioned in issue #28246 (moved)

mentioned in issue #31463 (moved)

moved to tpo/core/tor#27273 (closed)

mentioned in issue tpo/core/tor#28244 (closed)

mentioned in issue tpo/core/tor#28246 (closed)