Opened 4 months ago

Closed 3 months ago

#26196 closed defect (fixed)

Abort in test_bridges.c

Reported by: gvanem Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: crash, tests, 029-backport, 031-backport 032-backport 033-backport 034-backport fast-fix
Cc: catalyst Actual Points:
Parent ID: Points:
Reviewer: isis Sponsor:

Description

Running test.exe bridges/clear_bridge_list causes this assertion:

  bridges/clear_bridge_list: May 23 17:12:53.714 [err] tor_assertion_failed_():
  Bug: container.h:70: smartlist_get: Assertion sl->num_used > idx failed;

AFAICS, since sweep_bridge_list() caused all entries in bridgelist to get deleted, an index of zero is illegal. Don't ask me why.

And since I compiled all test/*.c sources with -DDEBUG_SMARTLIST, this will trigger this abort(). I fail to see why this isn't done in the officially.

PS1. I'm on Win-10, tor.exe + libs was built with MSVC 2017.

PS2. Build tor.exe from master at 23 May 2018.

Child Tickets

TicketStatusOwnerSummaryComponent
#26282closedrl1987Smartlist index implicitly converted to intCore Tor/Tor
#26283closedsmartlist_len cast to doubleCore Tor/Tor
#26284closedrl1987Out-of-bounds smartlist access in protover_compute_vote()Core Tor/Tor

Change History (15)

comment:1 Changed 4 months ago by teor

Cc: catalyst added
Component: - Select a componentCore Tor/Tor
Milestone: Tor: 0.3.5.x-final

This is a bug that we should fix: we should never access a smart list index beyond the used values.

But it's harmless in practice, because:

  • it's in the unit tests
  • smartlists are initialised with a minimum capacity of 16, and the allocation is zero-initialised
  • smartlist allocations never shrink
  • when elements are removed, the index is set to NULL

To prevent bugs like this in future, we should make unit tests and fragile hardening set DEBUG_SMARTLIST.

comment:2 in reply to:  1 Changed 4 months ago by catalyst

Replying to teor:

To prevent bugs like this in future, we should make unit tests and fragile hardening set DEBUG_SMARTLIST.

I agree with this approach. Also we should fix the test so it tests for zero length rather than checking for getting a null pointer from smartlist_get(bridgelist, 0) after the clear_bridge_list(). Unit tests generally shouldn't violate the invariants of code they're testing.

comment:3 Changed 4 months ago by nickm

Keywords: 029-backport 031-backport 032-backport 033-backport 034-backport added

I agree, and also we should backport this: we should treat this kind of thing as if it were a crash.

comment:4 Changed 4 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:5 Changed 4 months ago by rl1987

Status: acceptedneeds_review

comment:6 Changed 4 months ago by rl1987

Now that DEBUG_SMARTLIST is enabled when fragile hardening is on, there are more places in the codebase where we're using smartlist in a way that's not exactly right:

dir/v3_networkstatus: [forking] Jun 03 14:45:48.985 [err] tor_assertion_failed_: Bug: ./src/common/container.h:70: smartlist_get: Assertion sl->num_used > idx failed; aborting. (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.989 [err] Bug: Assertion sl->num_used > idx failed in smartlist_get at ./src/common/container.h:70. Stack trace: (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.990 [err] Bug:     0   test                                0x000000010d4e3df8 log_backtrace + 72 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.990 [err] Bug:     1   test                                0x000000010d55c461 tor_assertion_failed_ + 385 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.991 [err] Bug:     2   test                                0x000000010d2e276a smartlist_get + 490 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.991 [err] Bug:     3   test                                0x000000010d2e0d03 protover_compute_vote + 3315 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.992 [err] Bug:     4   test                                0x000000010d44a307 networkstatus_compute_consensus + 8823 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.992 [err] Bug:     5   test                                0x000000010cc7664a test_a_networkstatus + 13930 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.992 [err] Bug:     6   test                                0x000000010cf2b5b0 testcase_run_one + 880 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.993 [err] Bug:     7   test                                0x000000010cf2ba5f tinytest_main + 463 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.993 [err] Bug:     8   test                                0x000000010cf299ea main + 2218 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:48.994 [err] Bug:     9   libdyld.dylib                       0x00007fff6619f015 start + 1 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
[Lost connection!] 
  [v3_networkstatus FAILED]
dir/random_weighted: OK
dir/scale_bw: OK
dir/clip_unmeasured_bw_kb: [forking] Jun 03 14:45:49.151 [err] tor_assertion_failed_: Bug: ./src/common/container.h:70: smartlist_get: Assertion sl->num_used > idx failed; aborting. (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.157 [err] Bug: Assertion sl->num_used > idx failed in smartlist_get at ./src/common/container.h:70. Stack trace: (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.158 [err] Bug:     0   test                                0x000000010d4e3df8 log_backtrace + 72 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.158 [err] Bug:     1   test                                0x000000010d55c461 tor_assertion_failed_ + 385 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.159 [err] Bug:     2   test                                0x000000010d2e276a smartlist_get + 490 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.159 [err] Bug:     3   test                                0x000000010d2e0d03 protover_compute_vote + 3315 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.160 [err] Bug:     4   test                                0x000000010d44a307 networkstatus_compute_consensus + 8823 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.160 [err] Bug:     5   test                                0x000000010cc7664a test_a_networkstatus + 13930 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.161 [err] Bug:     6   test                                0x000000010cf2b5b0 testcase_run_one + 880 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.161 [err] Bug:     7   test                                0x000000010cf2ba5f tinytest_main + 463 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.161 [err] Bug:     8   test                                0x000000010cf299ea main + 2218 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.162 [err] Bug:     9   libdyld.dylib                       0x00007fff6619f015 start + 1 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
[Lost connection!] 
  [clip_unmeasured_bw_kb FAILED]
dir/clip_unmeasured_bw_kb_alt: [forking] Jun 03 14:45:49.196 [err] tor_assertion_failed_: Bug: ./src/common/container.h:70: smartlist_get: Assertion sl->num_used > idx failed; aborting. (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.200 [err] Bug: Assertion sl->num_used > idx failed in smartlist_get at ./src/common/container.h:70. Stack trace: (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.200 [err] Bug:     0   test                                0x000000010d4e3df8 log_backtrace + 72 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.200 [err] Bug:     1   test                                0x000000010d55c461 tor_assertion_failed_ + 385 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.201 [err] Bug:     2   test                                0x000000010d2e276a smartlist_get + 490 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.201 [err] Bug:     3   test                                0x000000010d2e0d03 protover_compute_vote + 3315 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.202 [err] Bug:     4   test                                0x000000010d44a307 networkstatus_compute_consensus + 8823 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.202 [err] Bug:     5   test                                0x000000010cc7664a test_a_networkstatus + 13930 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.202 [err] Bug:     6   test                                0x000000010cf2b5b0 testcase_run_one + 880 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.203 [err] Bug:     7   test                                0x000000010cf2ba5f tinytest_main + 463 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.203 [err] Bug:     8   test                                0x000000010cf299ea main + 2218 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:49.203 [err] Bug:     9   libdyld.dylib                       0x00007fff6619f015 start + 1 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
[Lost connection!] 
  [clip_unmeasured_bw_kb_alt FAILED]
protover/vote: Jun 03 14:45:59.046 [err] tor_assertion_failed_: Bug: ./src/common/container.h:70: smartlist_get: Assertion sl->num_used > idx failed; aborting. (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug: Assertion sl->num_used > idx failed in smartlist_get at ./src/common/container.h:70. Stack trace: (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     0   test                                0x000000010d4e3df8 log_backtrace + 72 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     1   test                                0x000000010d55c461 tor_assertion_failed_ + 385 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     2   test                                0x000000010d2e276a smartlist_get + 490 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     3   test                                0x000000010d2e0d03 protover_compute_vote + 3315 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     4   test                                0x000000010ce1cf13 test_protover_vote + 83 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     5   test                                0x000000010cf2b3d9 testcase_run_one + 409 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     6   test                                0x000000010cf2ba5f tinytest_main + 463 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     7   test                                0x000000010cf299ea main + 2218 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
Jun 03 14:45:59.051 [err] Bug:     8   libdyld.dylib                       0x00007fff6619f015 start + 1 (on Tor 0.3.4.1-alpha-dev 9168cfae11806184)
make: *** [test] Abort trap: 6

comment:7 Changed 4 months ago by teor

If your branch doesn't fix those assertion failures, please open more tickets so we don't forget them.

It looks like you're part of the way through the new tickets - thanks!

Last edited 4 months ago by teor (previous) (diff)

comment:8 Changed 4 months ago by nickm

Keywords: fast-fix added
Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final

comment:9 in reply to:  6 Changed 4 months ago by gvanem

Replying to rl1987:

Now that DEBUG_SMARTLIST is enabled when fragile hardening is on, there are more places in
the codebase where we're using smartlist in a way that's not exactly right:

Your patch to test/tor_bridges.c do indeed fix my original assertion. But as you've discovered, new assert() have surfaced. I'll leave that to you to fix.

comment:10 Changed 4 months ago by dgoulet

Reviewer: isis

comment:11 Changed 3 months ago by rl1987

Now that child tickets are all completed, I rebased the patch and created new pull request: https://github.com/torproject/tor/pull/154 Please review this one.

comment:12 Changed 3 months ago by isis

Status: needs_reviewmerge_ready

LGTM! Thanks rl1987!

comment:13 Changed 3 months ago by nickm

This is marked for backport as far as 0.2.9, but the branch is based on master, and the changes file says that the bug first appeared in 0.3.4.1-alpha.

Which branch should I actually merge this in? Should I cherry-pick the test_bridges.c change as far back as it will apply, and take the rest in master?

comment:14 Changed 3 months ago by rl1987

bridges/clear_bridge_list was introduced in c2c5b13e5d8a77eeee36028940175f182fda1ec9 and first shipped in 0.3.4.1-alpha. This specific patch should go to maint-0.3.4 and master. Child tickets did apply to earlier series of tor and were merged accordingly.

comment:15 Changed 3 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay, merged to 0.3.4 and forward!

Note: See TracTickets for help on using tickets.