Opened 11 months ago

Closed 5 months ago

#27273 closed defect (wontfix)

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

Reported by: alexcrichton Owned by:
Priority: High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust, 035-can
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 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/}

Child Tickets

Change History (21)

comment:1 Changed 11 months ago by nickm

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

comment:2 Changed 11 months ago by teor

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

comment:3 Changed 11 months ago by 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...

comment:4 Changed 11 months ago by teor

Parent ID: #25386

comment:5 in reply to:  3 Changed 10 months ago by cypherpunks3

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?

comment:6 Changed 10 months ago by alexcrichton

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 :(

comment:7 Changed 10 months ago by nickm

Priority: MediumHigh

comment:8 Changed 10 months ago by alexcrichton

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

comment:9 Changed 10 months ago by nickm

Status: newneeds_review

comment:10 Changed 9 months ago by dgoulet

Reviewer: teor

comment:11 Changed 9 months ago by nickm

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.

comment:12 Changed 9 months ago by nickm

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"

comment:13 Changed 9 months ago by 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'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

comment:14 in reply to:  13 Changed 9 months ago by teor

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.

comment:15 Changed 9 months ago by nickm

Status: needs_reviewneeds_revision

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.

Last edited 9 months ago by nickm (previous) (diff)

comment:16 Changed 9 months ago by alexcrichton

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.

comment:17 Changed 9 months ago by nickm

Resolution: fixed
Status: needs_revisionclosed

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 for the followup items that we identified above. Thank you!

comment:18 Changed 9 months ago by teor

Resolution: fixed
Status: closedreopened

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

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

comment:19 Changed 9 months ago by nickm

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.

comment:20 Changed 9 months ago by nickm

Keywords: 035-can added

comment:21 Changed 5 months ago by teor

Resolution: wontfix
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.