Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18241 closed enhancement (fixed)

Assert that the event_base is initialized before using it.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201602
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should assert that event_base is non-null in tor_libevent_get_base(), to avoid annoying bugs. (Libevent 2.0 will fail gently in this case, whereas libevent 1.4 will fail hard and confusingly).

Child Tickets

Change History (14)

comment:1 Changed 4 years ago by nickm

Owner: set to nickm
Status: newassigned

comment:2 Changed 4 years ago by nickm

Status: assignedneeds_review

Please review the one-line add-an-assertion thing in my public repository as branch assert_event_base.

comment:3 Changed 4 years ago by teor

This might break a whole lot of unit tests, as many of them don't initialise event_base, and then generate a whole lot of warnings about it being NULL.

(So we should fix the unit tests at the same time.)

comment:4 Changed 4 years ago by nickm

I think I did, with 1bac468882fd732460d8a25735131d632f977bfe

comment:5 Changed 4 years ago by teor

When I run make test with this branch checked out, I get an infinite recursion:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   test                          	0x000000010b5411b4 logv__real + 20 (log.c:472)
1   test                          	0x000000010b5422e0 log_fn_ + 144 (log.c:688)
2   test                          	0x000000010b541211 logv__real + 113 (log.c:487)
3   test                          	0x000000010b5422e0 log_fn_ + 144 (log.c:688)
4   test                          	0x000000010b541211 logv__real + 113 (log.c:487)
5   test                          	0x000000010b5422e0 log_fn_ + 144 (log.c:688)

Unfortunately, I can't work out where it starts, because the backtrace only goes down 512 stacks.

comment:6 Changed 4 years ago by teor

OK, it starts immediately after make runs ./src/test/test, and before any tests complete:

./src/test/test
make: *** [test] Segmentation fault: 11

comment:7 Changed 4 years ago by nickm

Very strange . Can you step into it, or tell me more about your platform? I didn't see that behavior when I ran my branch, and I tried it on OSX.

comment:8 Changed 4 years ago by teor

It happens on OS X 10.11.3 arch x86_64 using Apple LLVM version 7.0.2 (clang-700.1.81).
I'm using ./configure --disable-asciidoc --with-libevent-dir=/opt/local --with-openssl-dir=/opt/local.
This commit has issues, but the 0.2.8.1-alpha tarball in your volatile directory does not.
I can try to debug further.

comment:9 Changed 4 years ago by nickm

The infinite chain of log messages makes me think it's hitting an assertion failure before the logging subsystem is initialized. That makes us fail to get the logging lock, which makes us assert, which makes us try to log the assertion....

Hm. Maybe that's a bug too.

comment:10 Changed 4 years ago by teor

It turns out that the log mutex isn't initialised the first time logv() is called.
This causes logv to fail a tor_assert in LOCK_LOGS(), which calls logv()...

Please see my branch check_log_mutex for:

  • the check that exposes this issue,
  • further checks that ensure we don't log when SMARTLIST_DEBUG is defined,
  • a change to the initialiser order so we init logs first (should we do this in tor, too?)

I needed to cherry-pick 1bac468882fd732460d8a25735131d632f977bfe to get this branch to work.

tor_mutex_init() can still call tor_assert if it fails, so the possibility of this stack overflow still exists when we're initialising the log mutex itself.
What do you want to do about that one?

comment:11 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:12 Changed 4 years ago by nickm

Uncherrypicked the cherry-picked paert, and merged the rest to master. Could you please open another ticket for why we shouldn't assert from inside assert or log from inside log?

comment:13 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:14 Changed 4 years ago by teor

(See #18306 for the asserting-while-initialising issue.)

Note: See TracTickets for help on using tickets.