Link Rust Tests to C Dependencies in Tor (allow integration testing from Rust to C)

currently, it is not possible to call C Tor, directly or indirectly, from rust tests. one of the following must be done:

1. provide rust stubs for all C functions that may be needed for tests (impractical)

2. test rust functions from C (so we will have C tests calling Rust functions calling C functions)

3. link C functions into rust doctests (preferred)

4. never call C-using rust functions in tests (leads to poor test coverage, very bad)

my branch https://cgit.alxu.ca/tor.git/commit/?h=fix-rust-tests implements option 3 poorly. this is a bad solution firstly because it is very ugly, and secondly because it does not properly pass the system linking arguments, e.g. -L/opt/ssl. thirdly, it may hide problems in or cause to be compiled incorrectly dependency crates.

this ticket blocks a number of rust improvements, since of course we would like to actually test the improvements, and doctests are the best way to do it in rust.

changed milestone to %Tor: unspecified

added 033-unreached-backport 034-included-20180401 034-triage-20180328 035-removed-20180711 component::core tor/tor milestone::Tor: unspecified points::3 priority::high review-group-34 reviewer::isis rust severity::normal sponsor::Q status::new tor-test type::defect version::tor 0.3.3.1-alpha labels

Trac:
Type: enhancement to defect
Points: N/A to 1
Version: N/A to Tor: 0.3.2.1-alpha
Milestone: N/A to Tor: 0.3.4.x-final
Component: - Select a component to Core Tor/Tor
Keywords: N/A deleted, rust, tor-test added

Hello71 has a branch here: https://cgit.alxu.ca/tor.git/commit/?h=fix-rust-tests

But it needs to inherit libraries and library flags from the Makefile, And it needs to build Rust with asan, if asan is turned on.

We want to pass the -Z sanitizer flag to Rust: https://github.com/japaric/rust-san

Trac:
Status: new to needs_revision

still doesn't work with --enable-fragile-hardening due to ubsan, might work if that's commented out (or maybe not?)

I still think the best way is to build a "libtor.so" and link all the tests with it, so we don't have to pass all the dependencies in or interrogate the toolchain about how to link sanitizers.

Trac:
Status: needs_revision to needs_review

Let's get a minimal solution working first, and then look at ubsan and shared libraries.

Is the set of ".a" files minimal? Or should we use an environmental variable with that set of files instead? (For example, the set of tor source files linked into src/test/test would be a good place to start.)

Trac:
Keywords: N/A deleted, 033-backport added
Version: Tor: 0.3.2.1-alpha to Tor: 0.3.3.1-alpha

Trac:
Keywords: N/A deleted, review-group-34 added

Reviewer for week 03/19

Trac:
Reviewer: N/A to isis

Hello Hello71! (hah) Thanks a whole bunch for looking into this and for the patches! This looks much better than the attempts I was making going through cargo and rust #[cfg] linker directives.

Some notes on a couple issues:

1. I reverted 350f76d3 and 8748ddac in my bug25386 branch, as they both affected the TravisCI builds to enable OSX (which takes forever, hence why it's disabled by default) and also to (mostly?) disable clang (not sure why this was done? We like testing our C with different compilers, and clang's LeakSanitizer has caught leaks where GCC has not). Generally, in your personal repo, if you'd like to enable OSX builds for a branch, the way to do so is to make another branch off of it with the commits to enable the OSX builds, so that they don't accidentally get merged and make everyone's builds suddenly slow. (Also the changes seem to have resulted in building with GCC and --enable-cargo-online-mode twice somehow?)

2. Nick's make test-rust target from #25071 (moved) has been broken, due to no longer finding the test_rust.sh script.

3. make check fails. (That Travis build is just a copy of your branch that I prefixed with testing/hello71/ to keep it out of my branch namespace.) It looks like it's pretty angry about missing symbols. Locally, on my machine, building out-of-tree, it's mad about: {{{ FAIL: src/test/test_rust.sh

   error: could not find Cargo.toml in /home/isis/code/torproject/tor-build/src/rust or any parent directory}}} (Mytor/directory is at/home/isis/code/torproject/tor`.) This will [https://jenkins.torproject.org/ break our Jenkins builds].

I think once we're able to get these things fixed, then we can start on the UBSan/other sanitiser issues (possibly merging in-between).

Replying to isis:

Hello Hello71! (hah) Thanks a whole bunch for looking into this and for the patches! This looks much better than the attempts I was making going through cargo and rust #[cfg] linker directives.

Some notes on a couple issues:

1. I reverted 350f76d3 and 8748ddac in my bug25386 branch, as they both affected the TravisCI builds to enable OSX (which takes forever, hence why it's disabled by default) and also to (mostly?) disable clang (not sure why this was done? We like testing our C with different compilers, and clang's LeakSanitizer has caught leaks where GCC has not). Generally, in your personal repo, if you'd like to enable OSX builds for a branch, the way to do so is to make another branch off of it with the commits to enable the OSX builds, so that they don't accidentally get merged and make everyone's builds suddenly slow. (Also the changes seem to have resulted in building with GCC and --enable-cargo-online-mode twice somehow?)

2. Nick's make test-rust target from #25071 (moved) has been broken, due to no longer finding the test_rust.sh script.

3. make check fails. (That Travis build is just a copy of your branch that I prefixed with testing/hello71/ to keep it out of my branch namespace.) It looks like it's pretty angry about missing symbols. Locally, on my machine, building out-of-tree, it's mad about: {{{ FAIL: src/test/test_rust.sh

   error: could not find Cargo.toml in /home/isis/code/torproject/tor-build/src/rust or any parent directory}}} (Mytor/directory is at/home/isis/code/torproject/tor`.) This will break our Jenkins builds.

I think once we're able to get these things fixed, then we can start on the UBSan/other sanitiser issues (possibly merging in-between).

Oh, one more note:

4. This approach seems pretty sane, but one thing it won't allow is doing: {{{#!sh cd src/rust/protover cargo test }}} and getting a positive result. I'm not sure how much we should care about that, especially if—as a team—we're going to eventually agree that things will simply be rewritten in Rust, as opposed to maintained in both languages. (I.e. it's only a problem while we maintain both, and maintain the FFI boundary for both.)

Replying to isis:

1. I reverted 350f76d3 and 8748ddac in my bug25386 branch, as they both affected the TravisCI builds to enable OSX (which takes forever, hence why it's disabled by default) and also to (mostly?) disable clang (not sure why this was done? We like testing our C with different compilers, and clang's LeakSanitizer has caught leaks where GCC has not). Generally, in your personal repo, if you'd like to enable OSX builds for a branch, the way to do so is to make another branch off of it with the commits to enable the OSX builds, so that they don't accidentally get merged and make everyone's builds suddenly slow. (Also the changes seem to have resulted in building with GCC and --enable-cargo-online-mode twice somehow?)

This is why I don't like Travis's method of putting the configuration inside the repository. Here, I specifically want the changes to only apply to this branch. I suppose I could move that to another branch, and base this one on that one, but that sounds like a huge pain.

2. Nick's make test-rust target from #25071 (moved) has been broken, due to no longer finding the test_rust.sh script.

I will investigate.

3. make check fails. (That Travis build is just a copy of your branch that I prefixed with testing/hello71/ to keep it out of my branch namespace.) It looks like it's pretty angry about missing symbols.

Yes, I am aware, it doesn't work right with sanitizers. I discussed that with teor to some extent, but as I recall, I said "we should make a 'libtor.so'", he said "no, that will cause more problems", and I thought "I don't see any better way of doing it. I'll come back to it later." and then forgot to come back to it. I think there might be a solution in rustc -Csomething-or-other though.

Locally, on my machine, building out-of-tree, it's mad about: {{{ FAIL: src/test/test_rust.sh ===========================

error: could not find `Cargo.toml` in `/home/isis/code/torproject/tor-build/src/rust` or any parent directory`
}}}
(My `tor/` directory is at `/home/isis/code/torproject/tor`.) This will [break our Jenkins builds](https://jenkins.torproject.org/).

I think once we're able to get these things fixed, then we can start on the UBSan/other sanitiser issues (possibly merging in-between).

Ah, quite possible, I did not test this. I will investigate.

Replying to isis:

Oh, one more note:

4. This approach seems pretty sane, but one thing it won't allow is doing: {{{#!sh cd src/rust/protover cargo test }}} and getting a positive result. I'm not sure how much we should care about that, especially if—as a team—we're going to eventually agree that things will simply be rewritten in Rust, as opposed to maintained in both languages. (I.e. it's only a problem while we maintain both, and maintain the FFI boundary for both.)

This seems easy to rectify, just make test_rust.sh take a crate to test, or potentially check the current directory or something. In theory, we could do it in cargo too, but I don't know how to add libraries only during testing (or if such a thing exists).

Trac:
Status: needs_review to assigned
Owner: N/A to Hello71

Trac:
Status: assigned to needs_revision

Trac:
Sponsor: N/A to Sponsor8-can
Points: 1 to 3

Trac:
Cc: teor, isis to teor, isis, chelseakomlo

Hi, apologies for the late review, but is there a reason why the #3 (closed) option in the description is for doctests only? Ideally, we would want linking for at minimum integration tests, ideally all levels of tests.

Replying to chelseakomlo:

Hi, apologies for the late review, but is there a reason why the #3 (closed) option in the description is for doctests only? Ideally, we would want linking for at minimum integration tests, ideally all levels of tests.

er, I meant all tests. everything that gets run by cargo test.

Cool, thanks for clarifying and for working on this.

Another necessary criteria to think about for this effort is being able to run individual tests, or tests at a module level to ensure a quick feedback loop.

Fixed all problems except sanitizers not working. Asked #rust-beginners about building it using cargo, concluded it's too much work, and besides, scripts/cargo_test doesn't seem that much harder than cargo test. It would be nice to not duplicate the list of libraries, but I think that's a discussion for another time. this seems Good Enough for now.

Trac:
Status: needs_revision to needs_review
Version: Tor: 0.3.3.1-alpha to Tor: 0.3.3.2-alpha

I dunno how I changed the version, if it was automatic or maybe I scrolled there by accident. Change it back if it's important.

Commits are in chronological order for benefit of reviewers, I will rebase if approved. https://cgit.alxu.ca/tor.git/diff/?id2=master&id=fix-rust-tests for overview

Trac:
Version: Tor: 0.3.3.2-alpha to Tor: 0.3.3.1-alpha

Hi, this looks great! Thanks!

Just one tiny thing: when you squash, please remove commit f7b714ba411eb7ccdfcafe676df28f81bb823b57, as it breaks out-of-directory builds. (It should be $abs_top_srcdir in that line, since the test_rust.sh script doesn't get copied to the build dir.)

I added a commit on top in my bug25386_v2 branch, which changed the test you added to actually check the functionality of protover::compute_for_old_tor, and found a bug (#25605 (moved)), so this is pretty cool.

I recommend merging whenever Hello71 squashes, and then I'll put the tests I made/changed in #25605 (moved).

Trac:
Status: needs_review to merge_ready

Oh, also this needs a changes file. I can add that, if you like.

squashed and rebased on master (addition of --all-features flag). I only applied it in test_rust.sh; do you think it should be applied in cargo_test script instead? I think probably not, since I was trying to make cargo_test do basically the same thing as cargo test.

setting needs_review for that and a quick sanity check that I didn't break anything while squashing. no major changes in the overall diff though.

Trac:
Status: merge_ready to needs_review

rebased on master, added changes file

probably fixed non-system libraries

Oh, it also looks like your branch before squashing was failing when compiled with ASan? https://travis-ci.org/isislovecruft/tor/builds/357186429

Locally I was configuring with --enable-rust --enable-cargo-online-mode --enable-fatal-warnings, and forgot to use --enable-expensive-hardening.

Will check out the new branch and test that now.

Replying to isis:

Oh, it also looks like your branch before squashing was failing when compiled with ASan? https://travis-ci.org/isislovecruft/tor/builds/357186429

Locally I was configuring with --enable-rust --enable-cargo-online-mode --enable-fatal-warnings, and forgot to use --enable-expensive-hardening.

Will check out the new branch and test that now.

Okay, ASan is still failing here, and we discussed potential solutions in IRC a bit:

1. Additionally build a libtor.so (and libtool-ise it?) and link it in when compiling with cargo test.
2. Keep the original way that we're currently doing things in src/rust/test_rust.sh, but also add Hello71's branch here with a configure flag --enable-testing-c-from-rust (which sets --disable-expensive-hardening) and then we add a row/build to the CI build matrixes for additionally running that. (Potentially removing one of the cargo offline builds, since having two is a little overly-redundant… but that's a separate issue to this one.)
3. Other not-as-good ideas were discussed.

Any other ideas? Which of these sounds like a saner way forward?

Trac:
Status: needs_review to needs_revision

I think I agree that libtool is too much work. Possibly we could revisit that if we move to a better build system at some point.

I think I'll try my wrapper linker solution again and see how it works. If that doesn't work out, I'll do the solution where we do CI builds "no rust, ubsan + asan", "rust, asan only".

There is another problem though. The Rust code is not really well segmented into crates right now; it is possible for one crate to call indirectly code in another crate, through the magic of static linking. However, this does not work in tests, because there we only use Rust code from a single crate. Simply adding libtor_rust.a does not work, since then we get multiple definition errors. Presently, this is not an issue, because the linker automagically optimizes everything out with --gc-sections --as-needed. For some reason though, adding asan confuses the linker, and trying to test anything but protover causes undefined references to protover functions (because the C version is not compiled if Rust is enabled, but if we are testing Rust then we only add the current Rust crate). Moreover, eventually it is likely that we will want real interdependencies between crates, and I think there is little to no value to juggle Rust dependencies when there are no even hypothetical plans to embed them in something other than Tor proper all together. I think the best solution is to just merge all our Rust crates together, and I've filed #25639 (moved) to that effect. There may be better solutions, and I'm open to suggestions, but it seems to me like that is the only practical one.

Replying to Hello71:

I think I agree that libtool is too much work. Possibly we could revisit that if we move to a better build system at some point.

I think I'll try my wrapper linker solution again and see how it works. If that doesn't work out, I'll do the solution where we do CI builds "no rust, ubsan + asan", "rust, asan only".

Moreover, eventually it is likely that we will want real interdependencies between crates, and I think there is little to no value to juggle Rust dependencies when there are no even hypothetical plans to embed them in something other than Tor proper all together. I think the best solution is to just merge all our Rust crates together, and I've filed #25639 (moved) to that effect. There may be better solutions, and I'm open to suggestions, but it seems to me like that is the only practical one.

This isn't actually true. For example, we are currently in discussion about modularizing tor such that we can have multiple builds (i.e, for mobile clients, hidden services, etc). Being able to include/not include crates in select builds will be valuable for this effort. Furthermore, external projects will be able to leverage this as well, in a way that is not currently possible with tor's C code.

Moving all code into a single crate is a non-starter for me, as this goes against the larger effort of having tor be more modular in the future.

I would recommend looking at other Rust/C projects such as Gecko, talking to some Mozilla people, and thinking through something that both allows this functionality as well as enables us to have a clean/modular project structure into the future.

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

Doing something here will be needed to make progress with our rust code, which is needed for privcount.

Trac:
Keywords: 034-removed-20180328 deleted, 034-included-20180401 added
Sponsor: Sponsor8-can to SponsorQ

Trac:
Cc: teor, isis, chelseakomlo to teor, isis, chelseakomlo, manishearth@gmail.com

so it looks like building the shared library is actually the right way to go. asan applies only to the shared library in this case, can probably use -Z sanitizer=address on nightly for rust tests. everything actually works though, no inscrutable multiple definition errors. few hitches:

1. libtool sucks. currently using meson. would rather we switch to that entirely, but not opposed to someone else figuring out how to make libtor.so (basically tor minus tor_main.c) using libtool instead

2. still figuring out how best to link it in; hopefully will not use RUSTFLAGS so we don't have to recompile all dependencies on each test

I have pushed to https://cgit.alxu.ca/tor.git/log/?h=meson an implementation of the shared library approach (along with a bunch of other crap that I'll have to separate out). This approach seems to be the most reliable and, with -static-libasan, hopefully fixes all issues with asan incompatibilities. remaining problems:

1. still need to actually build a shared library, so either #23846 (moved) or switch to a better build system.

2. rustdoc doesn't take -Z sanitizer (https://github.com/rust-lang/rust/issues/43031), but I think maybe we can hack around that by cleaning first and just accept that doctests won't have sanitizers for now.

latest iteration:

1. still need to build a shared library

2. still needs cleanup, i.e. splitting commits. current state of code LGTM, good if people want to review Tor on meson vs autotools :)

3. needs testing on Mac and Windows (Rust should work on Windows now!)

4. cargo testing a single directory is once again unsupported. should be possible without too much work, but I need to think of a good interface. I think this is not a huge issue though, since cargo doesn't rebuild everything anymore. Definitely shouldn't block this bug, anyways.

5. everything works except rustdoc with asan due to aforementioned Rust bug. even Rust is built with asan, so it should catch everything asan can catch (within Rust limits, e.g. libstd is not instrumented). filed #25841 (moved) for actually checking that this works.

the basic architecture of this solution is fairly simple, and I tried to make minimal changes to the existing crate layout. it's basically 1. build a shared library, 2. get Rust to link with that shared library at compile-time, 3. use LD_LIBRARY_PATH so it can find it at run-time. the rest is finicky stuff to deal with Rust bugs like https://github.com/rust-lang/rust/issues/41807.

"minimal changes" means build scripts instead of links key in manifest. the latter might be preferable since it means the build scripts don't need to know anything about where they're being built. however, because links keys may not be duplicated in a dependency tree, that requires all Rust -> C definitions to be in a single crate, which is better handled in #25639 (moved).

It has occurred to me that shared libraries also split the global variable namespace along with the function namespace. This may be a problem.

I give up. Manish, what should we do here?

Replying to Hello71:

I give up. Manish, what should we do here?

Do you mind clarifying the outstanding blockers for this ticket? It is a bit hard to follow what has been resolved/is still outstanding (if the "latest iteration" comment is still valid, but there are recently-discovered blockers to closing this issue, more clarify on those blockers would be helpful).

so I realised that teor was right, you can't just have half of your program in a shared library. I think I was getting weird errors due to mixing gcc and llvm asan (different versions). I will try again hopefully this week with a shared library.

Sorry, yeah, I'm not very clear on what you need from me here.

(Apologies for not responding earlier, I was super busy and then completely forgot. Feel free to bug me here or on twitter if that happens again)

Trac:
Username: manish.earth

Hello71, or anybody: can you summarize what the actual problem is at this point? Chelseakomlo, Manish, and I are all confused about where we are.

It looks like the goal of this ticket is to make it so Tor's C code is linked with the Rust tests, so that Rust tests can call functions that invoke C functions. And it looks like we're running into linking issues. But, what are they? And what are we trying?

Trac:
Summary: fix rust tests to Link Rust Tests to C Dependencies in Tor (allow integration testing from Rust to C)

To summarize, I think the issues here (as I understand them) are these. I'll try to isolate them better and in more detail over the course of the week, but for now I'm just writing them down so I won't forget.

1. When we invoke cargo test, we need to make sure that RUSTFLAGS includes the -L and -l flags necessary to link our C code, including system libraries. We can do that in lots of different ways.

2. But doing this doesn't seem to actually pass the necessary flags to the linker when rustdoc is run. That's one problem. I think there have been a few bugs of this kind in the past, like https://github.com/rust-lang/cargo/issues/1401 .

3. Remaining problem is that when we have compiled our C code using -fsanitize=address and -fsanitize=undefined, we get linker issues. Adding these options to the linker line doesn't seem to help. Possibly we need to explicitly link with the backend files here, or somehow stop rustc from telling the linker -nodefaultlibs?

Okay, so (2) is specifically a problem with doctests only? Regular cargo test works fine?

Or is (3) a problem for regular cargo test as well?

Trac:
Username: manish.earth

My plan was originally to work on this instead of writing down the problems, but apparently I really don't want to work on it, so I'll write them down as I see them. This obsoletes most if not all of what I said above.

If we want to use cargo test to test things, then the following must be resolved:

1. Cargo needs to learn to link C Tor libraries in when running Rust tests. It's fine if the solution to this problem actually links C Tor libraries always instead of only during tests, because static libraries do not carry dependency information on any modern platform. (I believe not even Windows, but correct me if I'm wrong.) This can be accomplished a few ways:

a. set RUSTFLAGS and RUSTDOCFLAGS to "-L... -L... -L... -l... -l..." etc, either always or only when testing. the downside of this option is that it also links C Tor libraries when compiling Rust Tor dependencies, which sounds bad (but I suspect is actually fine if everything is linked together in "big bag of parts" manner at the end). always is better because cargo will save the value of these variables and recompile all Rust code (due to the previous problem). this isn't a huge issue now, but if we add a bunch of dependencies (like winapi, ugh) that could be an issue later.

b. do the same thing but specify rustflags in cargo configuration

c. use one or more build scripts that output all C Tor libraries as cargo:rustc-link-lib lines (make sure you add cargo:rustc-link-search too obviously). it is possible that using one in external only will work. I suspect we should rename and split external into tor_c and tor_c-sys, where the latter uses bindgen or manual curation for pure bindings only, and the former does high-level wrappers, but that can probably be done separately. I believe this is supposed to automatically work for rustdoc too.

d. use "links" key and provide the libraries in cargo configuration. this is possibly less preferable because it forecloses other uses of build scripts, like bindgen.

I am in favor of method c. It is possible that none of these work, however. This might be the case if Rust calls into C which calls back into the crate we are currently testing. If so, then a complicated build script might be necessary that excludes the current crate from the RUSTFLAGS. Additionally, note that "make a shared library" does not work, because then there will be multiple copies of global data structures.

2. sanitizers: if rustc is used to link tests (mandatory if using cargo test), then one of the following must be done (otherwise you will get "undefined reference" errors because the C code is looking for sanitizer symbols):

a. interpose the linker with -C linker to add -fsanitize options. I suspect -C link-arg won't work because of the weird order dependency of linker arguments. I also suspect that this will not work properly, especially if we want Rust tests to be run with sanitizers enabled.

b. pass -Z sanitizer options to Rust, either via exporting RUSTFLAGS or setting build.rustflags in cargo config. this requires that the Rust sanitizers are of a compatible version with the C sanitizers, otherwise there will be very strange errors, mostly multiple definition errors of __asan_* functions. usually, using clang to build C code is both necessary and sufficient. presently, gcc 8 should work with rustc 1.26, but probably not with newer Rust versions (gcc tends to trail behind on sanitizer versions).

3. a method should be devised if possible to test only one crate at a time. komlo asked for this somewhere above. probably a wrapper script is the best solution, but I'm not sure about the details. for example, should running it from the build directory be necessary? does it need to be autoconf generated, or can it be static?

Triaging some tickets not absolutely vital to 0.3.4 release to 0.3.5. These are fine to do in 0.3.4, but they aren't a must.

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final