Opened 2 years ago

Closed 2 years ago

#20715 closed defect (fixed)

memory leak in tor_cert_parse()

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: 028-backport review-group-13
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:


Running my Tor relay under valgrind with all the config options that Yawning's sandboxed Tor Browser turns on, I see:

==18287== 708 (288 direct, 420 indirect) bytes in 3 blocks are definitely lost in loss record 55 of 64
==18287==    at 0x4C28C20: malloc (in /usr/lib/valgrind/
==18287==    by 0x262FF7: tor_malloc_ (util.c:150)
==18287==    by 0x26308D: tor_malloc_zero_ (util.c:178)
==18287==    by 0x1B4972: tor_cert_parse (torcert.c:135)
==18287==    by 0x1A4F82: router_parse_entry_from_string (routerparse.c:2035)
==18287==    by 0x1A1907: routerlist_reparse_old (routerlist.c:3741)
==18287==    by 0x1A1907: update_consensus_router_descriptor_downloads (routerlist.c:5187)
==18287==    by 0x1A1C3F: update_all_descriptor_downloads (routerlist.c:4551)
==18287==    by 0x147FB2: launch_descriptor_fetches_callback (main.c:1475)
==18287==    by 0x166DBF: periodic_event_dispatch (periodic.c:52)
==18287==    by 0x53613DB: event_base_loop (in /usr/lib/x86_64-linux-gnu/
==18287==    by 0x14C1E3: run_main_loop_once (main.c:2385)
==18287==    by 0x14C1E3: run_main_loop_until_done (main.c:2429)
==18287==    by 0x14C1E3: do_main_loop (main.c:2357)
==18287==    by 0x14F934: tor_main (main.c:3486)

(This is the Tor from my bug20423 branch, which is commit caf742287 from close to the edge of maint-0.2.9 plus two hopefully irrelevant commits -- so I am calling the version

Child Tickets

Attachments (1)

0001-Fix-memory-leak-in-signed_descriptor_move.patch (727 bytes) - added by neel 2 years ago.
Patch to fix signed_descriptor_move() memory leak

Download all attachments as: .zip

Change History (12)

comment:1 Changed 2 years ago by arma

#19073 looks related but not obviously a culprit.

comment:2 Changed 2 years ago by arma

signed_descriptor_move() looks interesting here:

  ri = router_parse_entry_from_string(body,
                         0, 1, NULL, NULL);
  if (!ri)
    return NULL;
  signed_descriptor_move(&ri->cache_info, sd);

So in the router_parse_entry(), we make a new cert and assign it to

      router->cache_info.signing_key_cert = cert;

But then in signed_descriptor_move we

  memcpy(dest, src, sizeof(signed_descriptor_t));

Does that clobber the old ri->cache_info.signing_key_cert with whatever was in sd?

Changed 2 years ago by neel

Patch to fix signed_descriptor_move() memory leak

comment:3 Changed 2 years ago by neel

I'm not sure if the patch I submitted is everything necessary for this, but tell me what you think about this.

Neel Chauhan

comment:4 Changed 2 years ago by nickm

Status: newneeds_review

comment:5 Changed 2 years ago by nickm

Keywords: 028-backport added
Status: needs_reviewneeds_revision

Hi, neel! That won't work actually, since the dest pointer there can be a pointer to the inside of another allocation. (There are several structures that have signed_descriptor_t included in them.)

Maybe we should just copy the part that clears the relevant parts of the dest structure?

comment:6 Changed 2 years ago by nickm

Keywords: review-group-13 added

comment:7 Changed 2 years ago by nickm

Reviewer: nickm

comment:8 Changed 2 years ago by neel

I am new to contributing to Tor. Is it possible for you to give me more information on this so I can be able to track down the bug? (where to look, what functions, etc.)

comment:9 Changed 2 years ago by nickm

The issue here is that you can't free "dest" in the way that you do, since signed_descriptor_t might not be a pointer to a chunk that was returned by tor_malloc(). It might be part of a routerinfo_t structure, for example.

comment:10 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Ok I took this one as 029 is near stable. See branch bug20715_029_01 based on current release-0.2.9. This leak comes from 028 but I don't think I would backport it as this is not very security critical as far as I can tell.

@neel: If you have more questions about this, please don't hesitate to ask. I understand that I did made an "aggressive" takeover of this ticket but know that we value very much your contribution so if we can answer more questions, we'll be happy to do so!

comment:11 Changed 2 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good; merging!

FWIW, please remember that 0.2.9 fixups should go on maint-0.2.9

Note: See TracTickets for help on using tickets.