Memory leaks in handle_control_getconf

Found while investigating legacy/trac#33006 (moved):

=================================================================
==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)

changed milestone to %Tor: 0.4.3.x-final in legacy/trac

added 043-must in Legacy / Trac actualpoints::2 in Legacy / Trac component::core tor/tor in Legacy / Trac memory-leak in Legacy / Trac milestone::Tor: 0.4.3.x-final in Legacy / Trac owner::catalyst in Legacy / Trac points::1 in Legacy / Trac priority::medium in Legacy / Trac regression in Legacy / Trac resolution::fixed in Legacy / Trac reviewer::nickm in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tor-tests-coverage in Legacy / Trac type::defect in Legacy / Trac labels

Trac:
Owner: N/A to catalyst
Status: new to accepted

Trac:
Points: N/A to 1
Keywords: N/A deleted, tor-tests-coverage, memory-leak, regression added

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.

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?

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.

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.

Trac:
Actualpoints: N/A to 2
Status: accepted to needs_review

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?

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

Trac:
Reviewer: N/A to nickm

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

changed time estimate to 8h

added 16h of time spent

moved from legacy/trac#33039 (moved)

added Bug label and removed 1 deleted label

added Regression Tests labels and removed 1 deleted label