diff mbox series

[3/5,libbacktrace] Fix memory leak in loop in build_address_map

Message ID 20181128231653.GA3990@delia
State New
Headers show
Series [1/5,libbacktrace] Factor out backtrace_vector_free | expand

Commit Message

Tom de Vries Nov. 28, 2018, 11:16 p.m. UTC 
Hi,

When failing in build_address_map, we free the unit that's currently being
handled in the loop, but the ones that already have been allocated are leaked.

Fix this by keeping track of allocated units in a vector, and releasing them
upon failure.

Also, now that we have a vector of allocated units, move the freeing upon
failure of the abbrevs associated with each unit to build_address_map, and
remove the now redundant call to free_unit_addrs_vector.

Bootstrapped and reg-tested on x86_64.

OK for trunk?

Thanks,
- Tom

[libbacktrace] Fix memory leak in loop in build_address_map

2018-11-28  Ian Lance Taylor  <iant@golang.org>
	    Tom de Vries  <tdevries@suse.de>

	PR libbacktrace/88063
	* dwarf.c (free_unit_addrs_vector): Remove.
	(build_address_map): Keep track of allocated units in vector.  Free
	allocated units and corresponding abbrevs upon failure.  Remove now
	redundant call to free_unit_addrs_vector.  Free addrs vector upon
	failure.  Free allocated unit vector.

---
 libbacktrace/dwarf.c | 60 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

Comments

Li, Pan2 via Gcc-patches Dec. 27, 2018, 4:30 p.m. UTC | #1 
On Wed, Nov 28, 2018 at 3:17 PM Tom de Vries <tdevries@suse.de> wrote:
>
> When failing in build_address_map, we free the unit that's currently being
> handled in the loop, but the ones that already have been allocated are leaked.
>
> Fix this by keeping track of allocated units in a vector, and releasing them
> upon failure.
>
> Also, now that we have a vector of allocated units, move the freeing upon
> failure of the abbrevs associated with each unit to build_address_map, and
> remove the now redundant call to free_unit_addrs_vector.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk?
>
> Thanks,
> - Tom
>
> [libbacktrace] Fix memory leak in loop in build_address_map
>
> 2018-11-28  Ian Lance Taylor  <iant@golang.org>
>             Tom de Vries  <tdevries@suse.de>
>
>         PR libbacktrace/88063
>         * dwarf.c (free_unit_addrs_vector): Remove.
>         (build_address_map): Keep track of allocated units in vector.  Free
>         allocated units and corresponding abbrevs upon failure.  Remove now
>         redundant call to free_unit_addrs_vector.  Free addrs vector upon
>         failure.  Free allocated unit vector.

This is OK.

Thanks.

Ian
diff mbox series

Patch

diff --git a/libbacktrace/dwarf.c b/libbacktrace/dwarf.c
index b818911f5b4..8a7e94111ac 100644
--- a/libbacktrace/dwarf.c
+++ b/libbacktrace/dwarf.c
@@ -923,21 +923,6 @@  add_unit_addr (struct backtrace_state *state, uintptr_t base_address,
   return 1;
 }
 
-/* Free a unit address vector.  */
-
-static void
-free_unit_addrs_vector (struct backtrace_state *state,
-			struct unit_addrs_vector *vec,
-			backtrace_error_callback error_callback, void *data)
-{
-  struct unit_addrs *addrs;
-  size_t i;
-
-  addrs = (struct unit_addrs *) vec->vec.base;
-  for (i = 0; i < vec->count; ++i)
-    free_abbrevs (state, &addrs[i].u->abbrevs, error_callback, data);
-}
-
 /* Compare unit_addrs for qsort.  When ranges are nested, make the
    smallest one sort last.  */
 
@@ -1448,6 +1433,10 @@  build_address_map (struct backtrace_state *state, uintptr_t base_address,
 {
   struct dwarf_buf info;
   struct abbrevs abbrevs;
+  struct backtrace_vector units;
+  size_t units_count;
+  size_t i;
+  struct unit **pu;
 
   memset (&addrs->vec, 0, sizeof addrs->vec);
   addrs->count = 0;
@@ -1465,6 +1454,9 @@  build_address_map (struct backtrace_state *state, uintptr_t base_address,
   info.data = data;
   info.reported_underflow = 0;
 
+  memset (&units, 0, sizeof units);
+  units_count = 0;
+
   memset (&abbrevs, 0, sizeof abbrevs);
   while (info.left > 0)
     {
@@ -1503,10 +1495,20 @@  build_address_map (struct backtrace_state *state, uintptr_t base_address,
 
       addrsize = read_byte (&unit_buf);
 
+      pu = ((struct unit **)
+	    backtrace_vector_grow (state, sizeof (struct unit *),
+				   error_callback, data, &units));
+      if (pu == NULL)
+	  goto fail;
+
       u = ((struct unit *)
 	   backtrace_alloc (state, sizeof *u, error_callback, data));
       if (u == NULL)
 	goto fail;
+
+      *pu = u;
+      ++units_count;
+
       u->unit_data = unit_buf.buf;
       u->unit_data_len = unit_buf.left;
       u->unit_data_offset = unit_buf.buf - unit_data_start;
@@ -1531,27 +1533,33 @@  build_address_map (struct backtrace_state *state, uintptr_t base_address,
 				dwarf_ranges, dwarf_ranges_size,
 				is_bigendian, error_callback, data,
 				u, addrs))
-	{
-	  free_abbrevs (state, &u->abbrevs, error_callback, data);
-	  backtrace_free (state, u, sizeof *u, error_callback, data);
-	  goto fail;
-	}
+	goto fail;
 
       if (unit_buf.reported_underflow)
-	{
-	  free_abbrevs (state, &u->abbrevs, error_callback, data);
-	  backtrace_free (state, u, sizeof *u, error_callback, data);
-	  goto fail;
-	}
+	goto fail;
     }
   if (info.reported_underflow)
     goto fail;
 
+  // We only kept the list of units to free them on failure.  On
+  // success the units are retained, pointed to by the entries in
+  // addrs.
+  backtrace_vector_free (state, &units, error_callback, data);
+
   return 1;
 
  fail:
+  if (units_count > 0)
+    {
+      pu = (struct unit **) units.base;
+      for (i = 0; i < units_count; i++)
+	{
+	  free_abbrevs (state, &pu[i]->abbrevs, error_callback, data);
+	  backtrace_free (state, pu[i], sizeof **pu, error_callback, data);
+	}
+      backtrace_vector_free (state, &units, error_callback, data);
+    }
   free_abbrevs (state, &abbrevs, error_callback, data);
-  free_unit_addrs_vector (state, addrs, error_callback, data);
   if (addrs->count > 0)
     {
       backtrace_vector_free (state, &addrs->vec, error_callback, data);