Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#29552 closed defect (invalid)

memory leak: protover_contains_long_protocol_names in protover.c calls parse_protocol_list, but doesn't free smartlist returned (treats it as a boolean)

Reported by: drjohnson1984 Owned by:
Priority: High Milestone:
Component: Core Tor/Tor Version: Tor: 0.3.3.7
Severity: Normal Keywords: memory-leak
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The function parse_protocol_list in protover.c allocates a smartlist, and returns a pointer to the smartlist as long as there are no parse errors. The funcction protover_contains_long_protocol_names calls the former, but should capture and delete the returned smartlist if it is not null.

I searched the database for these function names and they didn't show up, so I assume this is unreported and still a problem. I apologize if I missed it somehow.

Child Tickets

Change History (3)

comment:1 Changed 7 months ago by dgoulet

Component: Core TorCore Tor/Tor
Priority: MediumHigh
Resolution: invalid
Status: newclosed

Here is the function:

bool
protover_contains_long_protocol_names(const char *s)
{
  smartlist_t *list = parse_protocol_list(s);
  if (!list)
    return true; /* yes, has a dangerous name */
  SMARTLIST_FOREACH(list, proto_entry_t *, ent, proto_entry_free(ent));
  smartlist_free(list);
  return false; /* no, looks fine */
}

The if (!list) means if (list == NULL) so we are looking at it and we only free it if non-NULL.

comment:2 Changed 7 months ago by drjohnson1984

The code looked like this in 0.3.3.7:
bool
protover_contains_long_protocol_names(const char *s)
{

if (!parse_protocol_list(s))

return true;

return false;

}

Because we modify source for internal testing and use, we only infrequently merge in versions. I figured this would have generated a bug report at some point to go with the fix, but I guess not. Sorry to have bothered you

comment:3 in reply to:  2 Changed 7 months ago by dgoulet

Replying to drjohnson1984:

The code looked like this in 0.3.3.7:

Ah! So 0.3.3.x is end of life in 7 days. I would strongly suggest to use at least 0.3.4 :).

It was backported in 0.3.3.8:

  o Major bugfixes (directory authority, backport from 0.3.4.3-alpha):
    - Stop leaking memory on directory authorities when planning to
      vote. This bug was crashing authorities by exhausting their
      memory. Fixes bug 26435; bugfix on 0.3.3.6.

See #26435.

Note: See TracTickets for help on using tickets.