memory leak in tor_cert_parse()

[arma]

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/vgpreload_memcheck-amd64-linux.so)
==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/libevent-2.0.so.5.1.9)
==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 0.2.9.5-alpha.)

[arma]

#19073 looks related but not obviously a culprit.

[arma]

signed_descriptor_move() looks interesting here:

  ri = router_parse_entry_from_string(body,
                         body+sd->signed_descriptor_len+sd->annotations_len,
                         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?

[neel]

Patch to fix signed_descriptor_move() memory leak

[neel]

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

Thanks,
Neel Chauhan

[nickm]

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?

[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.)

[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.

[dgoulet]

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!

[nickm]

Looks good; merging!

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