[patch] tcache double free check

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] tcache double free check

DJ Delorie-2

This patch uses a spare field in the tcache chunk overlay to store a
key, to detect if something is (or at least should be) in the tcache,
without having to iterate through the tcache.  While not perfect, it
allows us to detect many double free situations.  Not covered - if the
second free is by a different thread, because the second thread
wouldn't have visibility into the first thread's tcache (duh).

Includes test cases.  Detected one double free in elf/tst-leaks1,
which I also include a fix for.

        * malloc/malloc.c (tcache_entry): Add key field.
        (tcache_put): Check for double free in tcache.
        (tcache_get): Likewise.
        (_int_free): Likewise.
        * malloc/tst-tcfree1.c: New.
        * malloc/tst-tcfree2.c: New.
        * malloc/Makefile: Run the new tests.

        * dlfcn/dlerror.c (check_free): Prevent double frees.

diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
index 33574faab6..96bf925333 100644
--- a/dlfcn/dlerror.c
+++ b/dlfcn/dlerror.c
@@ -198,7 +198,10 @@ check_free (struct dl_action_result *rec)
       Dl_info info;
       if (_dl_addr (check_free, &info, &map, NULL) != 0 && map->l_ns == 0)
 #endif
- free ((char *) rec->errstring);
+ {
+  free ((char *) rec->errstring);
+  rec->errstring = NULL;
+ }
     }
 }
 
diff --git a/malloc/Makefile b/malloc/Makefile
index 7d54bad866..e6dfbfc14c 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -38,6 +38,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
  tst-malloc_info \
  tst-malloc-too-large \
  tst-malloc-stats-cancellation \
+ tst-tcfree1 tst-tcfree2 \
 
 tests-static := \
  tst-interpose-static-nothread \
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 67cdfd0ad2..beaab29a05 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2888,6 +2888,8 @@ mremap_chunk (mchunkptr p, size_t new_size)
 typedef struct tcache_entry
 {
   struct tcache_entry *next;
+  /* This field exists to detect double frees.  */
+  struct tcache_perthread_struct *key;
 } tcache_entry;
 
 /* There is one of these for each thread, which contains the
@@ -2911,6 +2913,27 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
 {
   tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
   assert (tc_idx < TCACHE_MAX_BINS);
+
+  /* This test succeeds on double free.  However, we don't 100% trust
+     it, so verify it's not an unlikely coincidence before
+     aborting.  */
+  if (__glibc_unlikely (e->key == tcache))
+    {
+      tcache_entry *tmp;
+      for (tmp = tcache->entries[tc_idx];
+   tmp;
+   tmp = tmp->next)
+ {
+ if (tmp == e)
+  malloc_printerr ("free(): double free detected in tcache");
+ }
+      /* If we get here, it was a coincidence.  We've wasted a few
+ cycles, but don't abort.  */
+    }
+  /* Now we mark this chunk as "in the tcache" so the above test will
+     detect a double free.  */
+  e->key = tcache;
+
   e->next = tcache->entries[tc_idx];
   tcache->entries[tc_idx] = e;
   ++(tcache->counts[tc_idx]);
@@ -2926,6 +2949,7 @@ tcache_get (size_t tc_idx)
   assert (tcache->entries[tc_idx] > 0);
   tcache->entries[tc_idx] = e->next;
   --(tcache->counts[tc_idx]);
+  e->key = NULL;
   return (void *) e;
 }
 
@@ -4173,6 +4197,21 @@ _int_free (mstate av, mchunkptr p, int have_lock)
  tcache_put (p, tc_idx);
  return;
       }
+    else
+      {
+ /* Check to see if it's already in the tcache.  */
+ tcache_entry *e = (tcache_entry *) chunk2mem (p);
+ /* Copy of test from tcache_put.  */
+ if (__glibc_unlikely (e->key == tcache && tcache))
+  {
+    tcache_entry *tmp;
+    for (tmp = tcache->entries[tc_idx];
+ tmp;
+ tmp = tmp->next)
+      if (tmp == e)
+ malloc_printerr ("free(): double free detected in tcache 2");
+  }
+      }
   }
 #endif
 
diff --git a/malloc/tst-tcfree1.c b/malloc/tst-tcfree1.c
new file mode 100644
index 0000000000..b1258f403f
--- /dev/null
+++ b/malloc/tst-tcfree1.c
@@ -0,0 +1,40 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <error.h>
+#include <limits.h>
+#include <malloc.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/signal.h>
+
+static int
+do_test (void)
+{
+  char * volatile x = malloc (32);
+
+  free (x); // puts in tcache
+  free (x); // should abort
+
+  printf("FAIL: tcache double free not detected\n");
+  return 1;
+}
+
+#define TEST_FUNCTION do_test
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>
diff --git a/malloc/tst-tcfree2.c b/malloc/tst-tcfree2.c
new file mode 100644
index 0000000000..2acc3036b5
--- /dev/null
+++ b/malloc/tst-tcfree2.c
@@ -0,0 +1,45 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <error.h>
+#include <limits.h>
+#include <malloc.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/signal.h>
+
+static int
+do_test (void)
+{
+  char * volatile ptrs[20];
+  int i;
+
+#define COUNT 20
+  for (i=0; i<COUNT; i++)
+    ptrs[i] = malloc (20);
+  for (i=0; i<COUNT; i++)
+    free (ptrs[i]);
+  free (ptrs[0]);
+
+  printf("FAIL: tcache double free\n");
+  return 1;
+}
+
+#define TEST_FUNCTION do_test
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

Florian Weimer-5
* DJ Delorie:

> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
> index 33574faab6..96bf925333 100644
> --- a/dlfcn/dlerror.c
> +++ b/dlfcn/dlerror.c
> @@ -198,7 +198,10 @@ check_free (struct dl_action_result *rec)
>        Dl_info info;
>        if (_dl_addr (check_free, &info, &map, NULL) != 0 && map->l_ns == 0)
>  #endif
> - free ((char *) rec->errstring);
> + {
> +  free ((char *) rec->errstring);
> +  rec->errstring = NULL;
> + }
>      }
>  }

This should go into a separate commit.  I'm not sure if this is the
right fix.  Why is check_free called twice?

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 67cdfd0ad2..beaab29a05 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2888,6 +2888,8 @@ mremap_chunk (mchunkptr p, size_t new_size)
>  typedef struct tcache_entry
>  {
>    struct tcache_entry *next;
> +  /* This field exists to detect double frees.  */
> +  struct tcache_perthread_struct *key;
>  } tcache_entry;
>  
>  /* There is one of these for each thread, which contains the
> @@ -2911,6 +2913,27 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
>  {
>    tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
>    assert (tc_idx < TCACHE_MAX_BINS);
> +
> +  /* This test succeeds on double free.  However, we don't 100% trust
> +     it, so verify it's not an unlikely coincidence before
> +     aborting.  */
> +  if (__glibc_unlikely (e->key == tcache))
> +    {
> +      tcache_entry *tmp;
> +      for (tmp = tcache->entries[tc_idx];
> +   tmp;
> +   tmp = tmp->next)
> + {
> + if (tmp == e)
> +  malloc_printerr ("free(): double free detected in tcache");
> + }
> +      /* If we get here, it was a coincidence.  We've wasted a few
> + cycles, but don't abort.  */
> +    }

Should the above land in a separate function?  The code below looks
pretty similar.

> +  /* Now we mark this chunk as "in the tcache" so the above test will
> +     detect a double free.  */
> +  e->key = tcache;

Does this put the address of the tcache control block on the heap?

>    e->next = tcache->entries[tc_idx];
>    tcache->entries[tc_idx] = e;
>    ++(tcache->counts[tc_idx]);
> @@ -2926,6 +2949,7 @@ tcache_get (size_t tc_idx)
>    assert (tcache->entries[tc_idx] > 0);
>    tcache->entries[tc_idx] = e->next;
>    --(tcache->counts[tc_idx]);
> +  e->key = NULL;
>    return (void *) e;
>  }

And this clears it again, so that it does not leak to user code, and
it's not necessary to check the entire list?  This should be mentioned
in a comment.

> +  free (x); // puts in tcache
> +  free (x); // should abort
> +
> +  printf("FAIL: tcache double free not detected\n");
> +  return 1;
> +}
> +
> +#define TEST_FUNCTION do_test
> +#define EXPECTED_SIGNAL SIGABRT
> +#include <support/test-driver.c>

This will print the malloc error message to the build output, where it
will confuse QE.  Perhaps you can use support_capture_subprocess to
capture the error message?

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

Florian Weimer-5
In reply to this post by DJ Delorie-2
* DJ Delorie:

> +  /* This test succeeds on double free.  However, we don't 100% trust
> +     it, so verify it's not an unlikely coincidence before
> +     aborting.  */
> +  if (__glibc_unlikely (e->key == tcache))
> +    {
> +      tcache_entry *tmp;
> +      for (tmp = tcache->entries[tc_idx];
> +   tmp;
> +   tmp = tmp->next)
> + {
> + if (tmp == e)
> +  malloc_printerr ("free(): double free detected in tcache");
> + }
> +      /* If we get here, it was a coincidence.  We've wasted a few
> + cycles, but don't abort.  */
> +    }

One more thing:

I think you should put an Systemtap probe here, perhaps counting the
length of the chain and logging the index, so that we can debug
performance issues here.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

Carlos O'Donell-6
On 11/8/18 3:09 PM, Florian Weimer wrote:

> * DJ Delorie:
>
>> +  /* This test succeeds on double free.  However, we don't 100% trust
>> +     it, so verify it's not an unlikely coincidence before
>> +     aborting.  */
>> +  if (__glibc_unlikely (e->key == tcache))
>> +    {
>> +      tcache_entry *tmp;
>> +      for (tmp = tcache->entries[tc_idx];
>> +   tmp;
>> +   tmp = tmp->next)
>> + {
>> + if (tmp == e)
>> +  malloc_printerr ("free(): double free detected in tcache");
>> + }
>> +      /* If we get here, it was a coincidence.  We've wasted a few
>> + cycles, but don't abort.  */
>> +    }
>
> One more thing:
>
> I think you should put an Systemtap probe here, perhaps counting the
> length of the chain and logging the index, so that we can debug
> performance issues here.

... and please document the probe in the manual.

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

DJ Delorie-2
In reply to this post by Florian Weimer-5

Florian Weimer <[hidden email]> writes:
>> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
>
> This should go into a separate commit.  I'm not sure if this is the
> right fix.  Why is check_free called twice?

I'm not sure either, but I included it because there would otherwise be
an unexplained regression triggered by my malloc patch, and I didn't
want to be blamed for it ;-)

>> +      /* If we get here, it was a coincidence.  We've wasted a few
>> + cycles, but don't abort.  */
>> +    }
>
> Should the above land in a separate function?  The code below looks
> pretty similar.

Perhaps an inline function.  I'm wary of adding more call overhead on
fast paths.

>> +  /* Now we mark this chunk as "in the tcache" so the above test will
>> +     detect a double free.  */
>> +  e->key = tcache;
>
> Does this put the address of the tcache control block on the heap?

Only while the blocks are in the tcache itself.  The value is arbitrary,
it can be anything that we can argue won't come up in usual program
flows.

> And this clears it again, so that it does not leak to user code, and
> it's not necessary to check the entire list?  This should be mentioned
> in a comment.

Right, the intention is that *if* the key has the magic value in it,
then we take the time to scan the whole list.

> This will print the malloc error message to the build output, where it
> will confuse QE.  Perhaps you can use support_capture_subprocess to
> capture the error message?

Or I could fprintf to stderr, but I was just following what the other
tests already do.
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

DJ Delorie-2
In reply to this post by Florian Weimer-5

Florian Weimer <[hidden email]> writes:
>> +      /* If we get here, it was a coincidence.  We've wasted a few
>> + cycles, but don't abort.  */
>> +    }
>
> One more thing:
>
> I think you should put an Systemtap probe here, perhaps counting the
> length of the chain and logging the index, so that we can debug
> performance issues here.

In tcache_get/tcache_put, or in this unlikely block?
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

Florian Weimer-5
* DJ Delorie:

> Florian Weimer <[hidden email]> writes:
>>> +      /* If we get here, it was a coincidence.  We've wasted a few
>>> + cycles, but don't abort.  */
>>> +    }
>>
>> One more thing:
>>
>> I think you should put an Systemtap probe here, perhaps counting the
>> length of the chain and logging the index, so that we can debug
>> performance issues here.
>
> In tcache_get/tcache_put, or in this unlikely block?

In this unlikely block.  Tcache instrumentation is a separate matter.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

DJ Delorie-2
In reply to this post by Florian Weimer-5

I decided to move the double free test to just _int_free, but before the
tcache_put call.  The test was always called anyway; moving this means
it's only coded in one place.  I added a probe that triggers when the
expensive scan happens, which effectively means "double free detected"
(or a one in a zillion chance meaning "I wasted some cycles" ;)

I'm still including the dlerror patch only to avoid regressions; I still
don't know for sure if that patch is the correct one for that error.

        * malloc/malloc.c (tcache_entry): Add key field.
        (tcache_put): Set it.
        (tcache_get): Likewise.
        (_int_free): Check for double free in tcache.
        * malloc/tst-tcfree1.c: New.
        * malloc/tst-tcfree2.c: New.
        * malloc/Makefile: Run the new tests.
        * manual/probes.texi: Document memory_tcache_double_free probe.

        * dlfcn/dlerror.c (check_free): Prevent double frees.

diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
index 33574faab6..96bf925333 100644
--- a/dlfcn/dlerror.c
+++ b/dlfcn/dlerror.c
@@ -198,7 +198,10 @@ check_free (struct dl_action_result *rec)
       Dl_info info;
       if (_dl_addr (check_free, &info, &map, NULL) != 0 && map->l_ns == 0)
 #endif
- free ((char *) rec->errstring);
+ {
+  free ((char *) rec->errstring);
+  rec->errstring = NULL;
+ }
     }
 }
 
diff --git a/malloc/Makefile b/malloc/Makefile
index 7d54bad866..e6dfbfc14c 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -38,6 +38,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
  tst-malloc_info \
  tst-malloc-too-large \
  tst-malloc-stats`-cancellation \
+ tst-tcfree1 tst-tcfree2 \
 
 tests-static := \
  tst-interpose-static-nothread \
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 67cdfd0ad2..0749d3fe70 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2888,6 +2888,8 @@ mremap_chunk (mchunkptr p, size_t new_size)
 typedef struct tcache_entry
 {
   struct tcache_entry *next;
+  /* This field exists to detect double frees.  */
+  struct tcache_perthread_struct *key;
 } tcache_entry;
 
 /* There is one of these for each thread, which contains the
@@ -2911,6 +2913,11 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
 {
   tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
   assert (tc_idx < TCACHE_MAX_BINS);
+
+  /* Mark this chunk as "in the tcache" so the test in _int_free will
+     detect a double free.  */
+  e->key = tcache;
+
   e->next = tcache->entries[tc_idx];
   tcache->entries[tc_idx] = e;
   ++(tcache->counts[tc_idx]);
@@ -2926,6 +2933,7 @@ tcache_get (size_t tc_idx)
   assert (tcache->entries[tc_idx] > 0);
   tcache->entries[tc_idx] = e->next;
   --(tcache->counts[tc_idx]);
+  e->key = NULL;
   return (void *) e;
 }
 
@@ -4166,6 +4174,26 @@ _int_free (mstate av, mchunkptr p, int have_lock)
   {
     size_t tc_idx = csize2tidx (size);
 
+    /* Check to see if it's already in the tcache.  */
+    tcache_entry *e = (tcache_entry *) chunk2mem (p);
+
+    /* This test succeeds on double free.  However, we don't 100%
+       trust it (it also matches random payload data at a 1 in
+       2^<size_t> chance), so verify it's not an unlikely coincidence
+       before aborting.  */
+    if (__glibc_unlikely (e->key == tcache && tcache))
+      {
+ tcache_entry *tmp;
+ LIBC_PROBE (memory_tcache_double_free, 2, e, tc_idx);
+ for (tmp = tcache->entries[tc_idx];
+     tmp;
+     tmp = tmp->next)
+  if (tmp == e)
+    malloc_printerr ("free(): double free detected in tcache 2");
+ /* If we get here, it was a coincidence.  We've wasted a few
+   cycles, but don't abort.  */
+      }
+
     if (tcache
  && tc_idx < mp_.tcache_bins
  && tcache->counts[tc_idx] < mp_.tcache_count)
diff --git a/malloc/tst-tcfree1.c b/malloc/tst-tcfree1.c
new file mode 100644
index 0000000000..b1258f403f
--- /dev/null
+++ b/malloc/tst-tcfree1.c
@@ -0,0 +1,40 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <error.h>
+#include <limits.h>
+#include <malloc.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/signal.h>
+
+static int
+do_test (void)
+{
+  char * volatile x = malloc (32);
+
+  free (x); // puts in tcache
+  free (x); // should abort
+
+  printf("FAIL: tcache double free not detected\n");
+  return 1;
+}
+
+#define TEST_FUNCTION do_test
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>
diff --git a/malloc/tst-tcfree2.c b/malloc/tst-tcfree2.c
new file mode 100644
index 0000000000..2acc3036b5
--- /dev/null
+++ b/malloc/tst-tcfree2.c
@@ -0,0 +1,45 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <error.h>
+#include <limits.h>
+#include <malloc.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/signal.h>
+
+static int
+do_test (void)
+{
+  char * volatile ptrs[20];
+  int i;
+
+#define COUNT 20
+  for (i=0; i<COUNT; i++)
+    ptrs[i] = malloc (20);
+  for (i=0; i<COUNT; i++)
+    free (ptrs[i]);
+  free (ptrs[0]);
+
+  printf("FAIL: tcache double free\n");
+  return 1;
+}
+
+#define TEST_FUNCTION do_test
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>
diff --git a/manual/probes.texi b/manual/probes.texi
index ab2a3102bb..0ea560ed78 100644
--- a/manual/probes.texi
+++ b/manual/probes.texi
@@ -243,6 +243,18 @@ This probe is triggered when the
 value of this tunable.
 @end deftp
 
+@deftp Probe memory_tcache_double_free (void *@var{$arg1}, int @var{$arg2})
+This probe is triggered when @code{free} determines that the memory
+being freed has probably already been freed, and resides in the
+per-thread cache.  Note that there is an extremely unlikely chance
+that this probe will trigger due to random payload data remaining in
+the allocated memory matching the key used to detect double frees.
+This probe actually indicates that an expensive linear search of the
+tcache, looking for a double free, has happened.  Argument @var{$arg1}
+is the memory location as passed to @code{free}, Argument @var{$arg2}
+is the tcache bin it resides in.
+@end deftp
+
 @node Mathematical Function Probes
 @section Mathematical Function Probes
 
Reply | Threaded
Open this post in threaded view
|

Re: [patch] tcache double free check

Carlos O'Donell-6
On 11/13/18 3:54 PM, DJ Delorie wrote:

>
> I decided to move the double free test to just _int_free, but before the
> tcache_put call.  The test was always called anyway; moving this means
> it's only coded in one place.  I added a probe that triggers when the
> expensive scan happens, which effectively means "double free detected"
> (or a one in a zillion chance meaning "I wasted some cycles" ;)
>
> I'm still including the dlerror patch only to avoid regressions; I still
> don't know for sure if that patch is the correct one for that error.
>
> * malloc/malloc.c (tcache_entry): Add key field.
> (tcache_put): Set it.
> (tcache_get): Likewise.
> (_int_free): Check for double free in tcache.
> * malloc/tst-tcfree1.c: New.
> * malloc/tst-tcfree2.c: New.
> * malloc/Makefile: Run the new tests.
> * manual/probes.texi: Document memory_tcache_double_free probe.
>
> * dlfcn/dlerror.c (check_free): Prevent double frees.

This looks good to me. Please commit with comment changes.

Reviewed-by: Carlos O'Donell <[hidden email]>

> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
> index 33574faab6..96bf925333 100644
> --- a/dlfcn/dlerror.c
> +++ b/dlfcn/dlerror.c
> @@ -198,7 +198,10 @@ check_free (struct dl_action_result *rec)
>        Dl_info info;
>        if (_dl_addr (check_free, &info, &map, NULL) != 0 && map->l_ns == 0)
>  #endif
> - free ((char *) rec->errstring);
> + {
> +  free ((char *) rec->errstring);
> +  rec->errstring = NULL;
> + }

OK. This is OK, because check_free can be called any number of times
in theory to check if the string can be freed. I think this is a regression
introduced by our freeres changes (where __dlerror_main_freeres frees it
and fini free it), where there was a double free but we
didn't detect it. Either way this is logically the correct change to make
here for this function. Tracking down why we call it more than once is not
important because the semantic is that we should be able to call it more than
once.

>      }
>  }
>  
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 7d54bad866..e6dfbfc14c 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -38,6 +38,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>   tst-malloc_info \
>   tst-malloc-too-large \
>   tst-malloc-stats`-cancellation \
> + tst-tcfree1 tst-tcfree2 \

OK.

>  
>  tests-static := \
>   tst-interpose-static-nothread \
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 67cdfd0ad2..0749d3fe70 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2888,6 +2888,8 @@ mremap_chunk (mchunkptr p, size_t new_size)
>  typedef struct tcache_entry
>  {
>    struct tcache_entry *next;
> +  /* This field exists to detect double frees.  */
> +  struct tcache_perthread_struct *key;

OK.

>  } tcache_entry;
>  
>  /* There is one of these for each thread, which contains the
> @@ -2911,6 +2913,11 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
>  {
>    tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
>    assert (tc_idx < TCACHE_MAX_BINS);
> +
> +  /* Mark this chunk as "in the tcache" so the test in _int_free will
> +     detect a double free.  */
> +  e->key = tcache;

OK. Add key.

> +
>    e->next = tcache->entries[tc_idx];
>    tcache->entries[tc_idx] = e;
>    ++(tcache->counts[tc_idx]);
> @@ -2926,6 +2933,7 @@ tcache_get (size_t tc_idx)
>    assert (tcache->entries[tc_idx] > 0);
>    tcache->entries[tc_idx] = e->next;
>    --(tcache->counts[tc_idx]);
> +  e->key = NULL;

OK. Remove key.

>    return (void *) e;
>  }
>  
> @@ -4166,6 +4174,26 @@ _int_free (mstate av, mchunkptr p, int have_lock)
>    {
>      size_t tc_idx = csize2tidx (size);
>  
> +    /* Check to see if it's already in the tcache.  */
> +    tcache_entry *e = (tcache_entry *) chunk2mem (p);
> +
> +    /* This test succeeds on double free.  However, we don't 100%
> +       trust it (it also matches random payload data at a 1 in
> +       2^<size_t> chance), so verify it's not an unlikely coincidence
> +       before aborting.  */
> +    if (__glibc_unlikely (e->key == tcache && tcache))
> +      {
> + tcache_entry *tmp;
> + LIBC_PROBE (memory_tcache_double_free, 2, e, tc_idx);

OK. Useful probe to look for e->key collision issues.

> + for (tmp = tcache->entries[tc_idx];
> +     tmp;
> +     tmp = tmp->next)
> +  if (tmp == e)
> +    malloc_printerr ("free(): double free detected in tcache 2");
> + /* If we get here, it was a coincidence.  We've wasted a few
> +   cycles, but don't abort.  */

OK.

> +      }
> +
>      if (tcache
>   && tc_idx < mp_.tcache_bins
>   && tcache->counts[tc_idx] < mp_.tcache_count)
> diff --git a/malloc/tst-tcfree1.c b/malloc/tst-tcfree1.c
> new file mode 100644
> index 0000000000..b1258f403f
> --- /dev/null
> +++ b/malloc/tst-tcfree1.c
> @@ -0,0 +1,40 @@

Please add one line test header here.

e.g. Test that malloc tcache catches double free.

> +/* Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <error.h>
> +#include <limits.h>
> +#include <malloc.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <sys/signal.h>
> +
> +static int
> +do_test (void)
> +{

Please add:

/* Do one allocation of any size that fits in tcache.  */

> +  char * volatile x = malloc (32);
> +
> +  free (x); // puts in tcache
> +  free (x); // should abort
> +
> +  printf("FAIL: tcache double free not detected\n");
> +  return 1;
> +}
> +
> +#define TEST_FUNCTION do_test
> +#define EXPECTED_SIGNAL SIGABRT

OK.

> +#include <support/test-driver.c>
> diff --git a/malloc/tst-tcfree2.c b/malloc/tst-tcfree2.c
> new file mode 100644
> index 0000000000..2acc3036b5
> --- /dev/null
> +++ b/malloc/tst-tcfree2.c
> @@ -0,0 +1,45 @@

Same as above, please add 1 line header.

> +/* Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <error.h>
> +#include <limits.h>
> +#include <malloc.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <sys/signal.h>
> +
> +static int
> +do_test (void)
> +{

Please explain why 20 is enough in a comment.

> +  char * volatile ptrs[20];
> +  int i;
> +
> +#define COUNT 20
> +  for (i=0; i<COUNT; i++)
> +    ptrs[i] = malloc (20);
> +  for (i=0; i<COUNT; i++)
> +    free (ptrs[i]);
> +  free (ptrs[0]);
> +
> +  printf("FAIL: tcache double free\n");
> +  return 1;
> +}
> +
> +#define TEST_FUNCTION do_test
> +#define EXPECTED_SIGNAL SIGABRT

OK.

> +#include <support/test-driver.c>
> diff --git a/manual/probes.texi b/manual/probes.texi
> index ab2a3102bb..0ea560ed78 100644
> --- a/manual/probes.texi
> +++ b/manual/probes.texi
> @@ -243,6 +243,18 @@ This probe is triggered when the
>  value of this tunable.
>  @end deftp
>  
> +@deftp Probe memory_tcache_double_free (void *@var{$arg1}, int @var{$arg2})
> +This probe is triggered when @code{free} determines that the memory
> +being freed has probably already been freed, and resides in the
> +per-thread cache.  Note that there is an extremely unlikely chance
> +that this probe will trigger due to random payload data remaining in
> +the allocated memory matching the key used to detect double frees.
> +This probe actually indicates that an expensive linear search of the
> +tcache, looking for a double free, has happened.  Argument @var{$arg1}
> +is the memory location as passed to @code{free}, Argument @var{$arg2}
> +is the tcache bin it resides in.

OK. Excellent text.

> +@end deftp
> +
>  @node Mathematical Function Probes
>  @section Mathematical Function Probes
>  
>


--
Cheers,
Carlos.