Opened 4 years ago

Closed 4 years ago

#18447 closed defect (fixed)

Possible double-free in test_options.c

Reported by: stevenc99 Owned by:
Priority: Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: validate__transproxy FreeBSD
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: None

Description

Hi,

On derivatives of FreeBSD that have net/pfvar.h, (GNU/kFreeBSD in my
case, but there will be others), USE_TRANSPARENT gets defined but
__FreeBSD__ is not.  Therefore when running options/validate__transproxy
in src/test/test_options.c:

   1080   free_options_test_data(tdata);

tdata remains a dangling pointer.  It may be assigned a new value in one
of the following ifdef blocks, which exist for linux, __FreeBSD__,
DARWIN and __OpenBSD__.  So in any other case when we reach:

   1115   free_options_test_data(tdata);

it would double-free the tdata from earlier.  I've attached a simple
patch to NULL that pointer the first time it is freed.

I will follow up with another ticket to enable transproxy on
GNU/kFreeBSD and enable this test to run on it.  Thanks.

Backtrace of the crash with -DNO_FORKING:

#0  routerset_free (routerset=0x21) at src/or/routerset.c:411
        cp_sl_idx = <optimized out>
        cp_sl_len = <optimized out>
        cp = <optimized out>
#1  0x000000000061d4e0 in or_options_free (options=0xae1ad0) at src/or/config.c:800
No locals.
#2  0x000000000051f3e5 in free_options_test_data (td=0xae2750) at src/test/test_options.c:391
No locals.
#3  0x00000000005231f3 in test_options_validate__transproxy (ignored=<optimized out>) at src/test/test_options.c:1115
        ret = <optimized out>
        tdata = 0xae2750
#4  0x00000000005ede8a in testcase_run_bare_ (testcase=0xaab430 <options_tests+400>) at src/ext/tinytest.c:106
        env = 0x0
        outcome = <optimized out>
#5  testcase_run_one (group=0xaa61e0 <testgroups+512>, testcase=0xaab430 <options_tests+400>) at src/ext/tinytest.c:253
        testcase = 0xaab430 <options_tests+400>
        group = 0xaa61e0 <testgroups+512>
#6  0x00000000005ee51e in tinytest_main (c=c@entry=3, v=v@entry=0x7fffffffe5b8, groups=0xaa5fe0 <testgroups>) at src/ext/tinytest.c:435
        i = 32
        j = 10
        n = <optimized out>
#7  0x000000000040d04b in main (c=3, v=0x7fffffffe5b8) at src/test/testing_common.c:300

Child Tickets

Attachments (2)

0001-test_options.c-NULL-a-pointer-after-free-18447.patch (820 bytes) - added by stevenc99 4 years ago.
0002-test_options.c-assert-that-TransProxyType-is-tested.patch (805 bytes) - added by stevenc99 4 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by rl1987

Sponsor: None
Status: newneeds_review

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:3 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Applied, and closing. Thanks!

For the other ticket, did #18448 work for you?

comment:4 Changed 4 years ago by stevenc99

Resolution: fixed
Status: closedreopened

Your commit for #18448 seems good to me, thanks.

Thanks for this too; although I don't see it yet in Git master?

What we also could do here, is assert that a platform-specific TransProxyType test did run (in this block which is guarded by USE_TRANSPARENT). That way if some new platform implements USE_TRANSPARENT, the test will deliberately fail until a TransProxyType test is written for that platform, so it isn't forgotten. See 0002-test_options.c-assert-that-TransProxyType-is-tested.patch. Thanks again!

comment:5 Changed 4 years ago by nickm

Status: reopenedneeds_review

Thanks for this too; although I don't see it yet in Git master?

Whoops; should be there now.

marking needs_review for the 2nd patch.

comment:6 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

looks fine; merged it. Thanks!

Note: See TracTickets for help on using tickets.