Opened 5 months ago

Closed 3 months ago

#22839 closed task (implemented)

Build tor with Rust code enabled on Windows

Reported by: isis Owned by: snoek
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust, windows, tor-build, review-group-22
Cc: isis, chelseakomlo, Sebastian, acrichton@… Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor: SponsorZ

Description

We should investigate building tor with ./configure --enable-rust on/for Windows. See the wiki page on Rust in Tor for more info. This is possibly a helpful endeavour for the Tor Browser Team as well, since the next Firefox ESR (released in March 2018 and finalised in June 2018) will not have an option to disable building with the Rust code.

Child Tickets

Change History (13)

comment:1 Changed 5 months ago by isis

Keywords: tor-build added

comment:2 Changed 5 months ago by dgoulet

Milestone: Tor: unspecified

comment:3 Changed 4 months ago by alexcrichton

Cc: acrichton@… added

comment:4 Changed 4 months ago by snoek

Owner: set to snoek
Status: newassigned

I'll have a crack at this. I'm not too versed in the Windows ecosystem, so if anyone with more experience was just about to work on this, I'm happy to pick up something else. It just seems like one of the more urgent things to work on right now, Rust-wise.

Last edited 4 months ago by snoek (previous) (diff)

comment:5 Changed 3 months ago by snoek

That was an interesting journey :)

tl;dr: Except for a gcc invocation reordering issue and a workaround for a possible Rust bug, things just work, as long as you have the right environment set up: MSYS2, to which we should probably migrate. I made two seperate commits for the rust changes and for what looks like a test bug. See below. I also opened a bug report on Rust for another issue, which I haven't yet heard a comment on.

The long story:

Very little work needed to be done on the Tor code proper to make Windows integration work:

  • I found one issue where the tor_util Rust object file on Windows depends on windows libraries, which were declared before the object file when invoking gcc.
  • The static library made by rustc using the MinGW toolchain had a MSVC suffix and prefix. So it was called tor_util.lib instead of libtor_util.a. I think this is a but in the rust compiler as .lib files and .a files don't have the same format. A few days ago I filed a bug for this against Rust (https://github.com/rust-lang/rust/issues/43749). To work around this I added an extra variable in configure.ac that should resolve this for now.

Here's the commit for these changes: https://github.com/rust-lang/rust/issues/43749

@alexcrichton, if you happen to read this, it just so happens that you fixed Rust bug 29508, which is the inverse of this possible Rust bug I described above in that MSVC static libs had the GCC .a naming. Any thoughts?

And also Rust itself needs to be setup correctly. This is basically a matter of using rustup, and configuring the target triple. This is all fairly standard Rust fare, but I think it's useful to have something up on the wiki. I can add this.

Getting Rust integration working did have some dependencies on how the environment is set up. Because Rust expects to use the MSYS2 GCC compiler (mingw-w64-i686-toolchain), which has better support for native Windows things. At the moment this just causes an issue with snprintf not being resolved, but in general we should be using the MSYS2 universe if our target is MinGW.

Which brings us to the fact that Tor is built with the olden MinGW/MSYS for its Jenkins builds. And I'm guessing these are used for that actual release artifacts. Also, of course other people outside of the Tor project proper might still be building Tor with some older non-MSYS2 Windows posix system. Another downside is that MSYS2 can't be installed on XP anymore.

So is moving towards MSYS2 acceptable? My guess would be yes. The old MinGW doesn't seem to be maintained much anymore, MSYS2 seems a lot nicer to work with (has an actual package manger Pacman, ported from Arch), and it has better Windows integration. But I'm not too versed in all of this.

As for testing this: I've compiled this for MSYS2 with and without Rust support, and things seemed to go fine mostly. I ran make check, and three tests didn't pass. Two didn't seem to have anything to do with my changes. One was definitely a 'Windows does pathnames backwards' bug (so only that one test failed in the main test test) and has happened on the Tor Jenkins builds as well. One failure happened in test_keygen.sh, ("Tor didn't declare that there would be no encryption"). This one happens on MSYS2 with and without Rust integration. Perhaps it was my env setup in some way. Haven't digged into this one yet, but it makes sense as being a separate bug.

The last test, test_memwipe, segfaulted. This only happened with Rust built in. It turns out this was caused by the uninitialized buf in check_heap_buffer being smaller than the mem addresses being scanned. I know we're doing some dirty stuff there, but I don't think trying to read past the length of the buffer was intended. At least to me it seems fair enough for the program to segfault. I put in the obvious fix, which might be horribly wrong.

commit for that fix: https://github.com/stuij/tor/commit/e6f552a184be8c5cee563e837dc33d9d038af88c

I don't forsee the Rust patch having any influence on environments that are not Windows, and on Windows, only a configuration with rust enabled should see any change, as in that it should now work, hopefully :)

Cross compiling from Linux to Windows should also work now if Rust is set up properly, but I haven't tested this. Before putting effort into this it seemed better to see what will happen with all of this.

Since configuring the build environment seems to happen out-of-tree (at least the trees that I found), obviously work still needs to be done to get a Rust compiler set up and configured for MinGW on Windows on the relevant boxes. That should go hand in hand with MSYS2 work.

comment:6 Changed 3 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Status: assignedneeds_review

Wow. That is a tremendous amount of work, and thank you for doing it!

I don't know any reason we shouldn't be able to update mingw/msys, but we should make sure that we do it in concert with our regular package builders. If possible, we shouldn't break people who try to use the old mingw without Rust, without giving them some time to upgrade. (But only if possible!)

The tor-memwipe.c fix looks fine to me. I think it's a potential backport as far as 0.2.8. (The bug seems to have been introduced in 0.2.7.2-alpha with commit 27bc0da14d1c9c91bfe31d88c862cd00972ab187, but 0.2.7.x isn't supported any more.)

comment:7 Changed 3 months ago by snoek

welcome :)

And yep, I think as long as we are not functionally dependent on Rust code, we shouldn't need to break things for the old/current MinGW. These changes above shouldn't break it. The only change that bubbles down into the makefile for non-rust building is the added windows userenv lib dependency, but that should come with any MinGW install.

comment:8 Changed 3 months ago by nickm

Keywords: review-group-22 added

comment:9 Changed 3 months ago by alexcrichton

@alexcrichton, if you happen to read this, it just so happens that you fixed Rust bug 29508, which is the inverse of this possible Rust bug I described above in that MSVC static libs had the GCC .a naming. Any thoughts?

Holy cow, turns out this has been the implementation since before 1.0! I actually had no idea about this...

That being said, I think that .a and .lib files are equivalent for the purposes of ld or w/e MinGW linker is being used (we just pass the .lib files to the linker). Have you found they don't work though? If they don't then we can surely get a fix in!

comment:10 Changed 3 months ago by alexcrichton

Er wait hang on, actually I think we used to emit libfoo.a and then https://github.com/rust-lang/rust/pull/29520/files caused an accidental regression here... In any case I'll continue discussion on https://github.com/rust-lang/rust/issues/43749

comment:11 Changed 3 months ago by nickm

I've opened a separate ticket for the test-memwipe fix, and backported the commit for it: #23291 .

The remaining commit to review here is 15bd1dba51cddceb526909e13c25be1990427541. I'm cherry-picking that one to master as 2e99f839e97e2b. Thanks!

Is this ticket ready to close?

comment:12 Changed 3 months ago by snoek

alexcrichton: thanks for the help/concern :) Yes, passing to the linker works.

nickm: I'm happy with closeage :)

comment:13 Changed 3 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

woo; closing. Thanks again!

Note: See TracTickets for help on using tickets.