Opened 5 months ago

Closed 4 months ago

#30674 closed defect (implemented)

Find out why ubsan/asan CI didn't catch #30629

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.4.1.1-alpha
Severity: Normal Keywords: 041-should, memory-safety, valgrind
Cc: nickm, dgoulet, teor, arma Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description


Child Tickets

Change History (7)

comment:1 Changed 5 months ago by nickm

Keywords: 041-should added; 041-must tor-events regression 041-regression removed
Owner: set to nickm
Status: newaccepted

comment:2 Changed 5 months ago by nickm

One theory proposed yesterday was that we didn't get a warning because libevent wasn't built with sanitizers.

comment:3 Changed 5 months ago by nickm

I can confirm that this does get caught if we build libevent with sanitizers, and link tor against that libevent -- which is not so easy.

I bet it would be worthwhile to add some kind of CI support for a "hardened compiler, hardened dependencies" build mode, but it would likely be a bit fragile. Or we could try to get our tests to support running everything under valgrind. Any thoughts here? I think our right move is to call this ticket solved, and open a new ticket for our chosen solution.

comment:4 Changed 5 months ago by nickm

Cc: teor arma added

comment:5 Changed 5 months ago by teor

Here are my initial thoughts:

Hardened dependencies:

  1. We know we can harden dependencies
  2. Hardened dependencies may cause CI failures due to bugs in dependencies
  3. Hardened dependencies may be slower
  4. We probably won't rebuild libc and other large libraries in hardened mode
  5. We don't know if valgrind or hardened builds provide better coverage of the kinds of coding errors we typically make
  6. It might be complicated to configure builds for all our dependencies
  7. We can't harden our chutney, stem, and sbws CIs, because they use pre-built binaries

Valgrind:

  1. We don't know if valgrind runs well in Travis CI
  2. Valgrind may cause CI failures due to bugs in dependencies
  3. Valgrind may be slower
  4. Valgrind instruments all the code, no matter which library it's in
  5. We don't know if valgrind or hardened builds provide better coverage of the kinds of coding errors we typically make
  6. Valgrind is simple to configure
  7. We can run valgrind on the pre-built binaries in our chutney, stem, and sbws CIs

if it works, and it's acceptably fast, valgrind is better than hardened builds.

So I think we should try valgrind in tor's CI, and see how well it works.
If it doesn't work, we should fall back to hardened builds.
If it works really well, we should consider using it in our other CIs.

comment:6 Changed 5 months ago by nickm

Parent ID: #30629

comment:7 in reply to:  5 Changed 4 months ago by nickm

Resolution: implemented
Status: acceptedclosed

I've opened #30801 to take action in response to the above actions and ideas; the goal of this ticket is complete.

Note: See TracTickets for help on using tickets.