Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4251 closed defect (fixed)

memory leak for hidden service?

Reported by: arma Owned by: rransom
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I ran moria1 under valgrind and it identified the following leak:

==29415== 80 bytes in 1 blocks are definitely lost in loss record 38 of 206
==29415==    at 0x4C244E8: malloc (vg_replace_malloc.c:236)
==29415==    by 0x4D3A07: _tor_malloc (util.c:144)
==29415==    by 0x4D4EC5: _tor_malloc_zero (util.c:170)
==29415==    by 0x42C579: rend_services_introduce (rendservice.c:1347)
==29415==    by 0x46CFB2: circuit_build_needed_circs (circuituse.c:708)
==29415==    by 0x40C644: second_elapsed_callback (main.c:1412)
==29415==    by 0x52C9343: event_base_loop (in /usr/lib/libevent-1.4.so.2.1.3)
==29415==    by 0x409990: do_main_loop (main.c:1889)
==29415==    by 0x409CCC: tor_main (main.c:2570)
==29415==    by 0x5F10C4C: (below main) (libc-start.c:228)

I haven't checked yet if it's a false positive, but usually there's something wrong in these cases. To be clear, moria1 runs a hidden service too.

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by arma

Here's a related (I think) hint:

==29415== 976 (16 direct, 960 indirect) bytes in 1 blocks are definitely lost in loss record 94 of 206
==29415==    at 0x4C244E8: malloc (vg_replace_malloc.c:236)
==29415==    by 0x4D3A07: _tor_malloc (util.c:144)
==29415==    by 0x4D6496: _crypto_new_pk_env_rsa (crypto.c:330)
==29415==    by 0x42C4AD: rend_services_introduce (rendservice.c:1948)
==29415==    by 0x46CFB2: circuit_build_needed_circs (circuituse.c:708)
==29415==    by 0x40C644: second_elapsed_callback (main.c:1412)
==29415==    by 0x52C9343: event_base_loop (in /usr/lib/libevent-1.4.so.2.1.3)
==29415==    by 0x409990: do_main_loop (main.c:1889)
==29415==    by 0x409CCC: tor_main (main.c:2570)
==29415==    by 0x5F10C4C: (below main) (libc-start.c:228)

I'm running git master.

comment:2 Changed 8 years ago by rransom

Owner: set to rransom
Status: newaccepted

comment:3 in reply to:  description ; Changed 8 years ago by rransom

Milestone: Tor: 0.2.2.x-final

Replying to arma:

I ran moria1 under valgrind and it identified the following leak:

==29415==    by 0x42C579: rend_services_introduce (rendservice.c:1347)
==29415==    by 0x46CFB2: circuit_build_needed_circs (circuituse.c:708)

src/or/rendservice.c:1347 is in rend_service_launch_establish_intro:

  launched->rend_data = tor_malloc_zero(sizeof(rend_data_t));

When a service-side intro circuit opens and we already have enough intro points for its HS, we set its purpose to CIRCUIT_PURPOSE_C_GENERAL (in rend_service_intro_has_opened), but do not free and clear its rend_data field. Fortunately, your Tor instance cannibalized that circuit for use as a hidden-service-related circuit and allocated a new rend_data for it, thereby leaking the old rend_data's memory, while it was running under valgrind. Nice catch!

Bugfix probably on 0.0.6, when hidden services were introduced, but I should check that before I put it in a changes/ file. Setting milestone to 0.2.2.x.

comment:4 in reply to:  1 Changed 8 years ago by rransom

Replying to arma:

Here's a related (I think) hint:

==29415==    by 0x42C4AD: rend_services_introduce (rendservice.c:1948)
      intro->intro_key = crypto_new_pk_env();

Looks like that cannibalized circuit whose rend_data leaked was turned into another intro circuit.

comment:5 in reply to:  3 Changed 8 years ago by rransom

Status: acceptedneeds_review

Replying to rransom:

Bugfix probably on 0.0.6, when hidden services were introduced, but I should check that before I put it in a changes/ file. Setting milestone to 0.2.2.x.

This turns out to be a bugfix on commit a5769eefa41ce9b7789b6868535e4d7b8fac86f0 (released in 0.2.1.7-alpha). See bug4251-022 ( https://git.torproject.org/rransom/tor.git bug4251-022 ) for a fix.

comment:6 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

subtle! I'll merge it. One question: what is up with the

   foo *p = thing->the_foo;
   thing->the_foo = NULL;
   foo_free(p);

pattern? Why not just

   foo_free(thing->the_foo);
   thing->the_foo = NULL;

?

It doesn't look bad to me, but I want to understand the point of it.

comment:7 in reply to:  6 Changed 8 years ago by rransom

Replying to nickm:

subtle! I'll merge it. One question: what is up with the

   foo *p = thing->the_foo;
   thing->the_foo = NULL;
   foo_free(p);

pattern? Why not just

   foo_free(thing->the_foo);
   thing->the_foo = NULL;

?

It doesn't look bad to me, but I want to understand the point of it.

It's a (bad) habit I picked up from programming in Delphi 5, to make the free-and-set-to-NULL process look almost thread-safe. (It's not thread-safe if other threads can ever write the variable.) I'll try to resist that temptation in the future, because there really is no point.

comment:8 Changed 7 years ago by nickm

Keywords: tor-hs added

comment:9 Changed 7 years ago by nickm

Component: Tor Hidden ServicesTor
Note: See TracTickets for help on using tickets.