#20987 closed defect (fixed)

Memory leak in rend_config_services()

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-14
Cc: teor Actual Points:
Parent ID: Points: .1
Reviewer: dgoulet Sponsor:

Description

options/validate__rend: [forking] 
=================================================================
==15407==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7f41e571be50 in malloc (/lib64/libasan.so.3+0xc6e50)
    #1 0x559322a5a86a in tor_malloc_ src/common/util.c:150
    #2 0x559322a32725 in smartlist_new__real src/common/container.c:34
    #3 0x559322911336 in rend_config_services src/or/rendservice.c:562
    #4 0x5593226fea9a in options_validate src/or/config.c:4012
    #5 0x559322505170 in test_options_validate__rend src/test/test_options.c:2741
    #6 0x559322626cf8 in testcase_run_bare_ src/ext/tinytest.c:106
    #7 0x5593226271f6 in testcase_run_forked_ src/ext/tinytest.c:190
    #8 0x5593226271f6 in testcase_run_one src/ext/tinytest.c:248
    #9 0x5593226286e5 in tinytest_main src/ext/tinytest.c:435
    #10 0x5593222de004 in main src/test/testing_common.c:313
    #11 0x7f41e2cef400 in __libc_start_main (/lib64/libc.so.6+0x20400)

Indirect leak of 128 byte(s) in 1 object(s) allocated from:
    #0 0x7f41e571be50 in malloc (/lib64/libasan.so.3+0xc6e50)
    #1 0x559322a5a86a in tor_malloc_ src/common/util.c:150
    #2 0x559322a5a911 in tor_malloc_zero_ src/common/util.c:178
    #3 0x559322a327c5 in smartlist_new__real src/common/container.c:37
    #4 0x559322911336 in rend_config_services src/or/rendservice.c:562
    #5 0x5593226fea9a in options_validate src/or/config.c:4012
    #6 0x559322505170 in test_options_validate__rend src/test/test_options.c:2741
    #7 0x559322626cf8 in testcase_run_bare_ src/ext/tinytest.c:106
    #8 0x5593226271f6 in testcase_run_forked_ src/ext/tinytest.c:190
    #9 0x5593226271f6 in testcase_run_one src/ext/tinytest.c:248
    #10 0x5593226286e5 in tinytest_main src/ext/tinytest.c:435
    #11 0x5593222de004 in main src/test/testing_common.c:313
    #12 0x7f41e2cef400 in __libc_start_main (/lib64/libc.so.6+0x20400)

SUMMARY: AddressSanitizer: 144 byte(s) leaked in 2 allocation(s).
OK

Child Tickets

Change History (10)

comment:1 Changed 20 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 20 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.2.9.x-final

comment:3 Changed 20 months ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final

comment:4 Changed 20 months ago by nickm

Cc: teor added
Status: acceptedneeds_review

Fix in branch bug20987 in my public repo.

Adding teor in cc since this leak was new in 8a0ea3ee43da006, so you probalby remember how the code goes. :)

comment:5 in reply to:  4 Changed 20 months ago by teor

Status: needs_reviewneeds_revision

Replying to nickm:

Fix in branch bug20987 in my public repo.

Adding teor in cc since this leak was new in 8a0ea3ee43da006, so you probalby remember how the code goes. :)

In general, the code in rend_config_services is complex and hard to modify correctly - it should be split into multiple functions. (Hopefully this will happen as part of the prop224 work!)

These cases have a double-free, they call rend_service_free(service) twice:

  • HiddenServiceAllowUnknownPorts
  • HiddenServiceDirGroupReadable
  • HiddenServiceMaxStreams
  • HiddenServiceMaxStreamsCloseCircuit
  • HiddenServiceNumIntroductionPoints

Is there a function for freeing a smartlist containing rend_service_t instances?
If there is, we should use it. If not, we should create it and use it.
(Perhaps in another ticket?)

comment:6 Changed 20 months ago by nickm

Status: needs_revisionneeds_review

I've tried to fix the double-free issue above in a new fixup commit on bug20987.

I'll open new tickets for cleanup when we merge the fix.

comment:7 Changed 20 months ago by nickm

Keywords: review-group-14 added

comment:8 Changed 19 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

And yes to address teor's concern, prop224 work is refactoring heavily that function and making it much less complex. It is one of the goal of this effort also that is simplify and better test the entire HS code base.

comment:9 Changed 19 months ago by dgoulet

Reviewer: dgoulet

comment:10 Changed 19 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

squashed and merged!

Note: See TracTickets for help on using tickets.