Opened 4 months ago

Closed 4 months ago

#26435 closed defect (fixed)

memory leak in parse_protocol_list

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 033-backport regression
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Running moria1 under valgrind, because all the dir auths have been OOMing lately, in hopes of finding the problem in action. Here's a promising one:

==41941== 48,407,360 (411,776 direct, 47,995,584 indirect) bytes in 25,736 blocks are definitely lost in loss record 79 of 79
==41941==    at 0x4A06A2E: malloc (vg_replace_malloc.c:270)
==41941==    by 0x2A8457: tor_malloc_ (util.c:150)
==41941==    by 0x299B6E: smartlist_new (container.c:34)
==41941==    by 0x172011: parse_protocol_list (protover.c:250)
==41941==    by 0x172108: protover_contains_long_protocol_names (protover.c:286)
==41941==    by 0x1C6CC1: dirserv_generate_networkstatus_vote_obj (dirvote.c:4378)
==41941==    by 0x1CDB1A: dirvote_act (dirvote.c:2893)
==41941==    by 0x15BDDA: dirvote_callback (main.c:2006)
==41941==    by 0x17153B: periodic_event_dispatch (periodic.c:55)
==41941==    by 0x50C6F8B: event_base_loop (in /usr/lib64/libevent-2.0.so.5.1.9)
==41941==    by 0x15B7EB: do_main_loop (main.c:3011)
==41941==    by 0x15BC1B: tor_run_main (main.c:4283)

Child Tickets

Change History (9)

comment:1 Changed 4 months ago by arma

Looks like protover_contains_long_protocol_names() calls parse_protocol_list(), checks if it succeeded, and returns, without freeing anything that it returned.

comment:2 Changed 4 months ago by nickm

Keywords: 033-backport regression added
Milestone: Tor: 0.3.4.x-final
Priority: MediumHigh

Looks like that was from our TROVE-2018-005 fix.

comment:3 Changed 4 months ago by arma

I've restarted moria1 with this patch:

diff --git a/src/or/protover.c b/src/or/protover.c
index e4efe0a..a02b9d1 100644
--- a/src/or/protover.c
+++ b/src/or/protover.c
@@ -283,9 +283,11 @@ parse_protocol_list(const char *s)
 bool
 protover_contains_long_protocol_names(const char *s)
 {
-  if (!parse_protocol_list(s))
-    return true;
-  return false;
+  smartlist_t *list = parse_protocol_list(s);
+  if (!list)
+    return true; /* yes, has a dangerous name */
+  smartlist_free(list);
+  return false; /* no, looks fine */
 }
 
 /**

comment:4 Changed 4 months ago by arma

Looks like I also want a

  SMARTLIST_FOREACH(protocols, proto_entry_t *, ent, proto_entry_free(ent));

in there. Probably we want a protocol_list_free or something to make it cleaner.

comment:5 Changed 4 months ago by arma

Keywords: 029-backport 032-backport added

comment:6 Changed 4 months ago by arma

Keywords: 029-backport 032-backport removed

Oh, I was misreading the ChangeLogs. Looks like #25517 only went into 0.3.3.6.

comment:7 Changed 4 months ago by arma

Status: newneeds_review

My bug26435 branch has a potential fix. It's based on maint-0.3.3.

I left the "refactor so there's a single function that frees a smartlist-of-protovers" idea for a later commit, maybe once nickm is done hacking on master.

I also see there is some rust stuff nearby this function, so somebody should be sure to look at that part too.

comment:8 Changed 4 months ago by nickm

lgtm; merged to 0.3.3 and forward.

comment:9 Changed 4 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.