Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Address Sanitizer (ASAN) configuration to Continuous Integration builds #8

Closed
briansmith opened this issue Sep 1, 2015 · 5 comments

Comments

@briansmith
Copy link
Owner

We should be doing runs with the tests running under ASAN.

briansmith pushed a commit that referenced this issue Mar 1, 2016
If d2i_PrivateKey hit the PKCS#8 codepath, it didn't enforce that the key was
of the specified type.

Note that this requires tweaking d2i_AutoPrivateKey slightly. A PKCS #8
PrivateKeyInfo may have 3 or 4 elements (optional attributes), so we were
relying on this bug for d2i_AutoPrivateKey to work.

Change-Id: If50b7a742f535d208e944ba37c3a585689d1da43
Reviewed-on: https://boringssl-review.googlesource.com/7253
Reviewed-by: Adam Langley <agl@google.com>
@joshuata
Copy link
Contributor

joshuata commented Feb 18, 2017

I have gotten a proof of concept working on macOS. It enables asan when built in DEBUG mode. There is still a ways to go, however.

The static libraries libring-core.a and libring-test.a must be linked against libasan.dylib if the program is compiled with clang. If it is compiled with gcc we can use the -static-libasan command-line flag to compile against the static version of the library. The location of libasan.dylib is platform and system dependent. Additionally, the name is different between macOS, iOS, watchOS, etc... I hardcoded it in build.rs in order to get something working, but it is far too brittle for a final implementation.

@briansmith
Copy link
Owner Author

Great to see. FWIW, there's no strong desire to support GCC on macOS, IMO.

@joshuata
Copy link
Contributor

The way I see it it makes sense to enable ASAN for a few targets, but not all. I think at least one gcc and one clang target would be wise (to take into account the differences in compiler passes and sanitizers) but I think that enabling it in every target would be too time consuming and fragile. I think a good start would be:

  • x86_64-apple-darwin, clang
  • x86_64-unknown-linux-gnu, gcc
  • x86_64-unknown-linux-gnu, clang
    I will evaluate ASAN under i686 to see whether it would be worth adding tests for that, but in general I think passing ASAN on one architecture should imply that you will pass it on all.

Would you prefer the option as a feature flag in the travis.yml file, a default target for debug mode, or something else?

@briansmith
Copy link
Owner Author

I will evaluate ASAN under i686 to see whether it would be worth adding tests for that.

I think that's a good idea because it will test 32-bit-specific code.

but in general I think passing ASAN on one architecture should imply that you will pass it on all.

50% of ring is architecture-specific code written in assembly language, so I think that that is less true for ring than for other Rust projects.

Note that we should pick one compiler version (probably the latest version of the compiler that is available on Travis CI) to do the sanitizer testing, instead of doing it for every compiler version we build on Travis CI with.

Also, keep in mind it is OK (better, even) to do things incrementally, maybe one platform at a time.

@joshuata
Copy link
Contributor

I've still been working on this, but it has been slow moving my existing work to the new build.rs script. I'm going to focus on linux gcc right now since it is the simplest platform to statically link with.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants