[PATCH v2 0/2] malloc: split the mtag realloc fix patch

[PATCH v2 0/2] malloc: split the mtag realloc fix patch

[Szabolcs Nagy]

i decided to split this patch:

 "[PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468]"
 https://sourceware.org/pipermail/libc-alpha/2021-March/123226.html

so in the second part i can make more changes, i need review
in that part.

rebased the rest of the patchset on top of this in nsz/mtag.

i'm working on follow up patches where this is useful (because
they touch chunk2mem).

Szabolcs Nagy (2):
  malloc: Fix a realloc crash with heap tagging [BZ 27468]
  malloc: Fix a potential realloc issue with memory tagging

 malloc/malloc.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] malloc: Fix a realloc crash with heap tagging [BZ 27468]

[Szabolcs Nagy]

_int_free must be called with a chunk that has its tag reset. This was
missing in a rare case that could crash when heap tagging is enabled:
when in a multi-threaded process the current arena runs out of memory
during realloc, but another arena still has space to finish the realloc
then _int_free was called without clearing the user allocation tags.

Fixes bug 27468.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 malloc/malloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 1f4bbd8edf..8f8f12c276 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3446,7 +3446,9 @@ __libc_realloc (void *oldmem, size_t bytes)
       newp = __libc_malloc (bytes);
       if (newp != NULL)
         {
-          memcpy (newp, oldmem, oldsize - SIZE_SZ);
+	  size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;
+	  memcpy (newp, oldmem, sz);
+	  (void) TAG_REGION (chunk2rawmem (oldp), sz);
           _int_free (ar_ptr, oldp, 0);
         }
     }
-- 
2.17.1

[PATCH v2 2/2] malloc: Fix a potential realloc issue with memory tagging

[Szabolcs Nagy]

At an _int_free call site in realloc the wrong size was used for tag
clearing: the chunk header of the next chunk was also cleared which
in practice may work, but logically wrong.

The tag clearing is moved before the memcpy to save a tag computation,
this avoids a chunk2mem.  Another chunk2mem is removed because newmem
does not have to be recomputed. Whitespaces got fixed too.
---
 malloc/malloc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 8f8f12c276..51cec67e55 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4851,14 +4851,14 @@ _int_realloc(mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize,
             }
           else
             {
-	      void *oldmem = chunk2mem (oldp);
+	      void *oldmem = chunk2rawmem (oldp);
+	      size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;
+	      (void) TAG_REGION (oldmem, sz);
 	      newmem = TAG_NEW_USABLE (newmem);
-	      memcpy (newmem, oldmem,
-		      CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ);
-	      (void) TAG_REGION (chunk2rawmem (oldp), oldsize);
-              _int_free (av, oldp, 1);
-              check_inuse_chunk (av, newp);
-              return chunk2mem (newp);
+	      memcpy (newmem, oldmem, sz);
+	      _int_free (av, oldp, 1);
+	      check_inuse_chunk (av, newp);
+	      return newmem;
             }
         }
     }
-- 
2.17.1

Re: [PATCH v2 2/2] malloc: Fix a potential realloc issue with memory tagging

[DJ Delorie]

Szabolcs Nagy via Libc-alpha <libc-alpha@sourceware.org> writes:
> At an _int_free call site in realloc the wrong size was used for tag
> clearing: the chunk header of the next chunk was also cleared which
> in practice may work, but logically wrong.
>
> The tag clearing is moved before the memcpy to save a tag computation,
> this avoids a chunk2mem.  Another chunk2mem is removed because newmem
> does not have to be recomputed. Whitespaces got fixed too.

LGTM.
Reviewed-by: DJ Delorie <dj@redhat.com>

(1/2 was already OK by me previously, and is unchanged)

> -	      void *oldmem = chunk2mem (oldp);
> +	      void *oldmem = chunk2rawmem (oldp);

Same but don't change the tag.  Ok.

> +	      size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;

This should be "chunk plus next header, minus next header and ours.".
I.e. user mem size.  Ok.

> 	      newmem = TAG_NEW_USABLE (newmem);
> -	      memcpy (newmem, oldmem,
> -		      CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ);
> -	      (void) TAG_REGION (chunk2rawmem (oldp), oldsize);

> +	      (void) TAG_REGION (oldmem, sz);
> +	      memcpy (newmem, oldmem, sz);

In both cases we're copying from oldmem, which ends up chunk-tagged
instead of user-tagged, into newmem, which is already self-tagged.  No
overlap is possible.  Ok.

> -              _int_free (av, oldp, 1);
> -              check_inuse_chunk (av, newp);
> -              return chunk2mem (newp);
> +	      _int_free (av, oldp, 1);
> +	      check_inuse_chunk (av, newp);
> +	      return newmem;

whitespace OK.