#26284 closed defect (fixed)

Out-of-bounds smartlist access in protover_compute_vote()

Reported by: rl1987 Owned by: rl1987
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: fast-fix
Cc: catalyst Actual Points:
Parent ID: #26196 Points:
Reviewer: asn Sponsor:

Description

When compiled with DEBUG_SMARTLIST:

dir/v3_networkstatus: [forking] Jun 03 15:26:03.147 [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 b32d8d6025fdc1be)
Jun 03 15:26:03.151 [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 b32d8d6025fdc1be)
Jun 03 15:26:03.151 [err] Bug:     0   test                                0x000000010ffdb108 log_backtrace + 72 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.152 [err] Bug:     1   test                                0x0000000110053771 tor_assertion_failed_ + 385 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.152 [err] Bug:     2   test                                0x000000010fdd9a7a smartlist_get + 490 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.152 [err] Bug:     3   test                                0x000000010fdd8013 protover_compute_vote + 3315 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.153 [err] Bug:     4   test                                0x000000010ff41617 networkstatus_compute_consensus + 8823 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.153 [err] Bug:     5   test                                0x000000010f76ac5a test_a_networkstatus + 13930 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.154 [err] Bug:     6   test                                0x000000010fa20684 testcase_run_bare_ + 308 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.154 [err] Bug:     7   test                                0x000000010fa203bc testcase_run_one + 2924 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.154 [err] Bug:     8   test                                0x000000010fa21e31 tinytest_main + 2321 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.155 [err] Bug:     9   test                                0x000000010fa1dffa main + 2218 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.155 [err] Bug:     10  libdyld.dylib                       0x00007fff6619f015 start + 1 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
Jun 03 15:26:03.155 [err] Bug:     11  ???                                 0x0000000000000001 0x0 + 1 (on Tor 0.3.4.1-alpha-dev b32d8d6025fdc1be)
[Lost connection!] 
  [v3_networkstatus FAILED]

Child Tickets

Change History (10)

comment:1 Changed 14 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:2 Changed 14 months ago by nickm

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

comment:3 Changed 14 months ago by rl1987

Status: acceptedneeds_review

comment:4 Changed 14 months ago by dgoulet

Reviewer: asn

comment:5 Changed 14 months ago by asn

Status: needs_reviewneeds_revision

Cool find rl1987! How did you repro this?

Do you think this needs to be backported? The way I understand it this can only trigger if all authorities don't participate in the protover protocol, which seems pretty unlikely.

Also I made some notes in the PR! Let me know how you like them!

comment:6 Changed 14 months ago by rl1987

This bug emerged when I was working on #26196. The simplest way to reproduce is to add #define DEBUG_SMARTLIST 1 to orconfig.h, recompile and run make test. There are several unit tests that crash in protover_compute_vote().

Last edited 14 months ago by rl1987 (previous) (diff)

comment:7 in reply to:  6 Changed 14 months ago by asn

Fixups LGTM.

Please squash the branch so that it's just one commit.

Also please consider adding some curly braces in this if clause:

+  if (smartlist_len(list_of_proto_strings) == 0)
+    return tor_strdup("");

IMO, we should be adding curly braces even in trivial if statements to avoid potential Apple goto fail issues ;)

After you do so, feel free to turn it into merge_ready.

Thanks! :)

comment:8 Changed 14 months ago by rl1987

Status: needs_revisionmerge_ready

comment:9 Changed 14 months ago by asn

LGTM!

comment:10 Changed 14 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.2.9.x-final
Resolution: fixed
Status: merge_readyclosed

Cherry-picked to 0.2.9, and merged forward!

Note: See TracTickets for help on using tickets.