diff mbox series

tcache double free check

Message ID xnr2fxtvqv.fsf@greed.delorie.com
State New
Headers show
Series tcache double free check | expand

Commit Message

DJ Delorie Nov. 7, 2018, 3:24 a.m. UTC 
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.

Comments

Florian Weimer Nov. 7, 2018, 8:52 a.m. UTC | #1 
* 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
Florian Weimer Nov. 8, 2018, 8:09 p.m. UTC | #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
Carlos O'Donell Nov. 8, 2018, 8:27 p.m. UTC | #3 
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.
diff mbox series

Patch

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>