Opened 5 weeks ago

Closed 4 weeks ago

#33039 closed defect (fixed)

Memory leaks in handle_control_getconf

Reported by: nickm Owned by: catalyst
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 043-must, memory-leak, regression, tor-tests-coverage
Cc: Actual Points: 2
Parent ID: Points: 1
Reviewer: nickm Sponsor:

Description

Found while investigating #33006:

=================================================================
==180005==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7fb54c5cdc58 in __interceptor_malloc (/lib64/libasan.so.5+0x10dc58)
    #1 0x55a4af274f3a in tor_malloc_ src/lib/malloc/malloc.c:45
    #2 0x55a4af274fd0 in tor_malloc_zero_ src/lib/malloc/malloc.c:71
    #3 0x55a4af2371ca in config_line_append src/lib/encoding/confline.c:40
    #4 0x55a4aeffaec7 in control_reply_add_one_kv src/feature/control/control_proto.c:345
    #5 0x55a4aefdd2ae in handle_control_getconf src/feature/control/control_cmd.c:307
    #6 0x55a4aefe54b2 in handle_single_control_command src/feature/control/control_cmd.c:2376
    #7 0x55a4aefe54b2 in handle_control_command src/feature/control/control_cmd.c:2407
    #8 0x55a4aefd6e01 in connection_control_process_inbuf src/feature/control/control.c:508
    #9 0x55a4aeee0e01 in connection_handle_read_impl src/core/mainloop/connection.c:3737
    #10 0x55a4aeee0e01 in connection_handle_read src/core/mainloop/connection.c:3777
    #11 0x55a4aeeecf00 in conn_read_callback src/core/mainloop/mainloop.c:892
    #12 0x7fb54c32abb6  (/lib64/libevent-2.1.so.6+0x25bb6)

Indirect leak of 26 byte(s) in 1 object(s) allocated from:
    #0 0x7fb54c55652d in strdup (/lib64/libasan.so.5+0x9652d)
    #1 0x55a4af2751d0 in tor_strdup_ src/lib/malloc/malloc.c:165
    #2 0x55a4af2371d5 in config_line_append src/lib/encoding/confline.c:41
    #3 0x55a4aeffaec7 in control_reply_add_one_kv src/feature/control/control_proto.c:345
    #4 0x55a4aefdd2ae in handle_control_getconf src/feature/control/control_cmd.c:307
    #5 0x55a4aefe54b2 in handle_single_control_command src/feature/control/control_cmd.c:2376
    #6 0x55a4aefe54b2 in handle_control_command src/feature/control/control_cmd.c:2407
    #7 0x55a4aefd6e01 in connection_control_process_inbuf src/feature/control/control.c:508
    #8 0x55a4aeee0e01 in connection_handle_read_impl src/core/mainloop/connection.c:3737
    #9 0x55a4aeee0e01 in connection_handle_read src/core/mainloop/connection.c:3777
    #10 0x55a4aeeecf00 in conn_read_callback src/core/mainloop/mainloop.c:892
    #11 0x7fb54c32abb6  (/lib64/libevent-2.1.so.6+0x25bb6)

Indirect leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x7fb54c55652d in strdup (/lib64/libasan.so.5+0x9652d)
    #1 0x55a4af2751d0 in tor_strdup_ src/lib/malloc/malloc.c:165
    #2 0x55a4af2371f5 in config_line_append src/lib/encoding/confline.c:42
    #3 0x55a4aeffaec7 in control_reply_add_one_kv src/feature/control/control_proto.c:345
    #4 0x55a4aefdd2ae in handle_control_getconf src/feature/control/control_cmd.c:307
    #5 0x55a4aefe54b2 in handle_single_control_command src/feature/control/control_cmd.c:2376
    #6 0x55a4aefe54b2 in handle_control_command src/feature/control/control_cmd.c:2407
    #7 0x55a4aefd6e01 in connection_control_process_inbuf src/feature/control/control.c:508
    #8 0x55a4aeee0e01 in connection_handle_read_impl src/core/mainloop/connection.c:3737
    #9 0x55a4aeee0e01 in connection_handle_read src/core/mainloop/connection.c:3777
    #10 0x55a4aeeecf00 in conn_read_callback src/core/mainloop/mainloop.c:892
    #11 0x7fb54c32abb6  (/lib64/libevent-2.1.so.6+0x25bb6)

SUMMARY: AddressSanitizer: 65 byte(s) leaked in 3 allocation(s).
Exit code was 1
 success (15.21s)

Child Tickets

Change History (9)

comment:1 Changed 5 weeks ago by catalyst

Owner: set to catalyst
Status: newaccepted

comment:2 Changed 5 weeks ago by catalyst

Keywords: memory-leak regression tor-tests-coverage added
Points: 1

comment:3 Changed 5 weeks ago by catalyst

Having trouble reproducing this on Xenial with manual testing of issuing GETCONF to the control port. clang LeakSanitizer doesn't seem to detect a leak even when the incorrect frees are deleted. Trying on Bionic.

comment:4 Changed 5 weeks ago by nickm

I haven't tested this, but it looks like maybe the cleanup code in getconf is incorrect? It's doing:

  SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp));

but I don't think that the "answers" smartlist is actually a list of strings?

comment:5 in reply to:  4 Changed 5 weeks ago by catalyst

Replying to nickm:

I haven't tested this, but it looks like maybe the cleanup code in getconf is incorrect? It's doing:

  SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp));

but I don't think that the "answers" smartlist is actually a list of strings?

Thanks, that's what I meant by deleting the incorrect frees. I'm pretty sure the mismatch in free functions is the problem; I just can't get LeakSanitizer to show the leaks, even though it shows them for a minimal test program. Also Xenial Bionic isn't helping. Going to try valgrind on Xenial Bionic.

Last edited 5 weeks ago by catalyst (previous) (diff)

comment:6 Changed 5 weeks ago by catalyst

Actual Points: 2
Status: acceptedneeds_review

Pull request in https://github.com/torproject/tor/pull/1684

As a bonus, unit test coverage in handle_control_getconf() is now at 100%.

I did confirm that the new unit tests correctly detect the leak under valgrind on Ubuntu Bionic. (valgrind doesn't work with OpenSSL on Xenial, so I had to install a new VM.) For some reason, AddressSanitizer doesn't catch the leaks in tor itself, but does catch them in the test program. (Both Xenial and Bionic, and I think both clang and gcc.) I wish we could understand why.

comment:7 Changed 5 weeks ago by nickm

This looks solid to me, and CI is passing. Thanks! I'm merging it now.

Please close this ticket, after un-parenting or closing the child ticket as appropriate?

comment:8 Changed 5 weeks ago by nickm

Reviewer: nickm

This looks solid to me, and CI is passing. Thanks! I'm merging it now.

Please close this tickete, after un-parenting the child or

comment:9 Changed 4 weeks ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.