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: 0.2.9.5-alpha |
Severity: | Normal | Keywords: | 028-backport review-group-13 |
Cc: | Actual Points: | ||
Parent ID: | Points: | ||
Reviewer: | nickm | Sponsor: |
Description
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.)
Child Tickets
Attachments (1)
Change History (12)
comment:1 Changed 2 years ago by
comment:2 Changed 2 years ago by
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?
Changed 2 years ago by
Attachment: | 0001-Fix-memory-leak-in-signed_descriptor_move.patch added |
---|
Patch to fix signed_descriptor_move() memory leak
comment:3 Changed 2 years ago by
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
comment:4 Changed 2 years ago by
Status: | new → needs_review |
---|
comment:5 Changed 2 years ago by
Keywords: | 028-backport added |
---|---|
Status: | needs_review → needs_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
Keywords: | review-group-13 added |
---|
comment:7 Changed 2 years ago by
Reviewer: | → nickm |
---|
comment:8 Changed 2 years ago by
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
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
Status: | needs_revision → needs_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
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
Looks good; merging!
FWIW, please remember that 0.2.9 fixups should go on maint-0.2.9
#19073 looks related but not obviously a culprit.