diff mbox series

malloc: Validate tc_idx before checking for double-frees in tcache [BZ #23907]

Message ID 87zhu1z1ja.fsf@oldenburg.str.redhat.com
State New
Headers show
Series malloc: Validate tc_idx before checking for double-frees in tcache [BZ #23907] | expand

Commit Message

Florian Weimer Nov. 22, 2018, 1:26 p.m. UTC 
The previous check could read beyond the end of the tcache entry
array.  If the e->key == tcache cookie check happened to pass, this
would result in crashes.

2018-11-22  Florian Weimer  <fweimer@redhat.com>

	[BZ #23907]
	* malloc/malloc.c (_int_free): Validate tc_idx before checking for
	double-frees.

Comments

Florian Weimer Nov. 22, 2018, 2:53 p.m. UTC | #1 
* Florian Weimer:

> The previous check could read beyond the end of the tcache entry
> array.  If the e->key == tcache cookie check happened to pass, this
> would result in crashes.
>
> 2018-11-22  Florian Weimer  <fweimer@redhat.com>
>
> 	[BZ #23907]
> 	* malloc/malloc.c (_int_free): Validate tc_idx before checking for
> 	double-frees.

One more note.  This check

> +	/* 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))

makes it difficult to write a regression test for this because we cannot
easily determine the tcache cookie value from the test.  Otherwise we
could use that to spray the heap and likely trigger this issue quite
reliably.

Thanks,
Florian
diff mbox series

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index f730d7a2ee..c9b2c6e320 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4225,33 +4225,33 @@  _int_free (mstate av, mchunkptr p, int have_lock)
 #if USE_TCACHE
   {
     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))
+    if (tcache != NULL && tc_idx < mp_.tcache_bins)
       {
-	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.  */
-      }
+	/* Check to see if it's already in the tcache.  */
+	tcache_entry *e = (tcache_entry *) chunk2mem (p);
 
-    if (tcache
-	&& tc_idx < mp_.tcache_bins
-	&& tcache->counts[tc_idx] < mp_.tcache_count)
-      {
-	tcache_put (p, tc_idx);
-	return;
+	/* 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_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->counts[tc_idx] < mp_.tcache_count)
+	  {
+	    tcache_put (p, tc_idx);
+	    return;
+	  }
       }
   }
 #endif