Opened 3 years ago

Closed 3 years ago

#20710 closed defect (fixed)

memory leak in sandbox_getaddrinfo()

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

Description

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

==18287== 125 (48 direct, 77 indirect) bytes in 1 blocks are definitely lost in loss record 30 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 0x270682: sandbox_getaddrinfo (sandbox.c:1530)
==18287==    by 0x250718: tor_addr_lookup (address.c:273)
==18287==    by 0x256F44: tor_lookup_hostname (compat.c:2649)
==18287==    by 0x1E9E0D: resolve_my_address (config.c:2421)
==18287==    by 0x18E154: router_pick_published_address (router.c:1980)
==18287==    by 0x190EAE: router_rebuild_descriptor (router.c:2337)
==18287==    by 0x190FE5: router_get_my_routerinfo (router.c:1912)
==18287==    by 0x191659: router_get_my_descriptor (router.c:1924)
==18287==    by 0x191AEE: init_keys (router.c:981)

To complicate things slightly, 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

Change History (8)

comment:1 Changed 3 years ago by arma

Initial poking leads me to

void
sandbox_free_getaddrinfo_cache(void)
{
  cached_getaddrinfo_item_t **next, **item;

  for (item = HT_START(getaddrinfo_cache, &getaddrinfo_cache);
       item;
       item = next) {
    next = HT_NEXT_RMV(getaddrinfo_cache, &getaddrinfo_cache, item);
    cached_getaddrinfo_item_free(*item);
  }

  HT_CLEAR(getaddrinfo_cache, &getaddrinfo_cache);
}

You see the HT_NEXT_RMV in the middle? I think it updates *item to point to the next element. So when we call cached_getaddrinfo_item_free on it, it's freeing the wrong one. Or freeing some random thing in memory, more likely.

Compare to how we call it in container.c, which is

      this = *ent;                                                      \
      next = HT_NEXT_RMV(prefix##_impl, &map->head, ent);               \
      if (free_val)                                                     \
        free_val(this->val);                                            \

comment:2 Changed 3 years ago by cypherpunks

Version: Tor: 0.2.9.5-alphaTor: 0.2.5.5-alpha

The function was added in commit e425fc78045f99725d256956acc7360ed71bfaa5 which was included in version 0.2.5.5-alpha. Using a temporary variable to store the current item seems to be the solution.

diff --git a/src/common/sandbox.c b/src/common/sandbox.c
index 24ba8a2..ebc843e 100644
--- a/src/common/sandbox.c
+++ b/src/common/sandbox.c
@@ -1579,13 +1579,14 @@ sandbox_add_addrinfo(const char *name)
 void
 sandbox_free_getaddrinfo_cache(void)
 {
-  cached_getaddrinfo_item_t **next, **item;
+  cached_getaddrinfo_item_t **next, **item, *this;
 
   for (item = HT_START(getaddrinfo_cache, &getaddrinfo_cache);
        item;
        item = next) {
+    this = *item;
     next = HT_NEXT_RMV(getaddrinfo_cache, &getaddrinfo_cache, item);
-    cached_getaddrinfo_item_free(*item);
+    cached_getaddrinfo_item_free(this);
   }
 
   HT_CLEAR(getaddrinfo_cache, &getaddrinfo_cache);

comment:3 Changed 3 years ago by arma

Agreed.

Now, is this just a memory leak, or is that old cached_getaddrinfo_item_free() dangerous in some way?

That is, can we convince ourselves that when it does

    *elm = (*elm)->field.hte_next;                                      \

inside HT_NEXT_RMV, *elm isn't dangerous to try to free?

Fortunately, sandbox_free_getaddrinfo_cache() is only called on fork and on shutdown.

comment:4 in reply to:  3 Changed 3 years ago by cypherpunks

Replying to arma:

That is, can we convince ourselves that when it does

    *elm = (*elm)->field.hte_next;                                      \

inside HT_NEXT_RMV, *elm isn't dangerous to try to free?

It does look dangerous because item == elm, item is freed and in the next iteration is read from as this snippet shows.

comment:5 Changed 3 years ago by nickm

Keywords: 025-backport 026-backport 027-backport 028-backport added
Status: newmerge_ready

I've added a changes file and put the code for review in a bug20710_025 branch. I think we can merge it, if it looks ok to y'all.

comment:6 Changed 3 years ago by nickm

Keywords: review-group-13 added

comment:7 Changed 3 years ago by cypherpunks

LGTM

comment:8 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merging to 0.2.9 and forward only, since this is a leak-at-exit.

Note: See TracTickets for help on using tickets.