Opened 5 years ago

Closed 5 years ago

#15817 closed enhancement (fixed)

Allow clang runtime sanitizers to be used on tor unit tests

Reported by: teor Owned by: teor
Priority: Low Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: unit-tests clang
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The clang runtime address sanitizer causes the memwipe and backtrace tests to fail, because it catches the undefined behavior that is invoked as an unavoidable part of these tests.

This issue can be resolved by:

  1. blacklisting the involved functions so the sanitizer doesn't check them, by adding to the command line -fsanitize-blacklist=sanitize_blacklist.txt containing:
    # test-memwipe.c checks if a freed buffer was properly wiped
    fun:vmemeq
    
    # test_bt_cl.c stores to a NULL pointer to trigger a crash
    fun:crash
    
    # we need to exempt the entire file, otherwise address sanitizer munges
    # the expected output
    src:test_bt_cl.c
    
  2. allowing the backtrace handler to catch SIGSEGV (rather than address sanitizer), by setting the environmental variable ASAN_OPTIONS=allow_user_segv_handler=1

The attached file is a sanitizer blacklist file which documents this resolution, and implements the blacklisting (but not the environmental variable).

Nick, can we put this in contrib?
I think that's the best way to deal with the special case of "clang users running tests under AddressSanitizer"

Child Tickets

Attachments (1)

sanitize_blacklist_tests.txt (749 bytes) - added by teor 5 years ago.
clang AddressSanitizer blacklist for tor unit tests

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by teor

clang AddressSanitizer blacklist for tor unit tests

comment:1 Changed 5 years ago by teor

Status: newneeds_information
Type: defectenhancement

comment:2 Changed 5 years ago by nickm

Is there any good way we could have this apply to only the two tests that invoke crashes/undefined behavior? I'd like to have all the sanitizers run on the other tests.

comment:3 Changed 5 years ago by teor

For test-memwipe.c, we can run the sanitizers on the entire executable, but exempt the function which uses undefined behaviour using a blacklist with fun:vmemeq in it.

For test_bt_cl.c, we can run the sanitizers on the entire executable, but exempt the function which uses undefined behaviour using a blacklist with fun:crash in it. We also need to pass ASAN_OPTIONS=allow_user_segv_handler=1 in the environment to allow tor's custom backtrace handler to run.

These changes won't impact the other tests.

Is that what you meant?

comment:4 Changed 5 years ago by teor

Also, did you want these in contrib?

It seems a waste to add the arguments to every clang command-line, even when the sanitizers aren't being used.
Or are you suggesting we turn on the sanitizers for every test run under clang?
The only issue I can see with this is that the sanitizer invocation syntax is a nightmare, it is still under active development and changes markedly between releases.

comment:5 Changed 5 years ago by nickm

Is that what you meant?

That sounds good, yes.

Also, did you want these in contrib?

It sounds good; either contrib/something or doc/HACKING or somewhere. We document how to use other tools in doc/HACKING/ -- maybe this will fit in?

comment:6 Changed 5 years ago by teor

Owner: set to teor
Status: needs_informationaccepted

How about I prepare a sanitizer blacklist file to go in contrib/clang, and then document it in doc/HACKING?

Now that I know how it works, I'll also prepare another blacklist for curve25519-donna.c, as it also uses undefined behaviour. (At least until we fix #13538.)

That way, all the known undefined behaviour in the codebase is documented in the one place.

I'll work on a patch for it.

comment:7 Changed 5 years ago by teor

Status: acceptedassigned

comment:8 Changed 5 years ago by teor

Status: assignedneeds_review

Branch: feature15817-clang-sanitizers
Repository: https://github.com/teor2345/tor.git
Changes:

  • add coverity, clang static analyzer, and clang dynamic sanitizers to doc/HACKING
  • add contrib/clang/sanitizer_blacklist.txt including detailed usage instructions
  • includes changes file

Tested with clang 3.7's UndefinedBehaviorSanitizer and AddressSanitizer on OS X 10.10 Yosemite, with make check and make test-network passing on both x86_64 and i386 (using CC="clang -arch i386").

comment:9 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

looks nice; merged!

comment:10 Changed 5 years ago by teor

Resolution: implemented
Status: closedreopened

I updated the branch feature15817-clang-sanitizers to:

  • add some compatibility notes for make and ccache
  • change to using function names, rather than source file paths. This should allow the blacklist to be used with out-of-tree builds.

This is only a minor change to the contrib/clang/sanitizer_blacklist.txt file.

comment:11 Changed 5 years ago by teor

Status: reopenedneeds_review

comment:12 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged!

Note: See TracTickets for help on using tickets.