malloc: Always call memcpy in _int_realloc [BZ #24027]

Message ID 87sgyluz8t.fsf@mid.deneb.enyo.de
State New
Headers show
Series
  • malloc: Always call memcpy in _int_realloc [BZ #24027]
Related show

Commit Message

Florian Weimer Dec. 25, 2018, 8:29 p.m.
This patch removes the custom memcpy implementation from _int_realloc
for small chunk sizes.  ncopies has the wrong type and could thus
result in too few elements being copied, so removing this code fixes
bug 24027.  I don't think the inlining is performance-critical because
this code is used when the realloc results in an _int_malloc, copy,
and _int_free, so even for small allocations there is quote some
overhead beyond the copy itself.

I'm not sure if this warrants tracking as a security bug.  Looking at
the code, the problem could be trigger in a default configuration if
mremap fails and a subsequent mmap succeeds.

2018-12-23  Florian Weimer  <fw@deneb.enyo.de>

	[BZ #24027]
	* malloc/malloc.c (_int_realloc): Always call memcpy for the
	copying operation.

Comments

Niklas Hamb├╝chen Dec. 25, 2018, 8:55 p.m. | #1
Would it not make sense to first apply the patch I provided in

  https://sourceware.org/bugzilla/show_bug.cgi?id=24027

that fixes the type, and then do the removal of the memcpy special case as a refactoring?

That would ensure that if somebody disagrees with the performance-critical bit in the future and reverts this, they don't revert back to the bugged version.
It would also make it possible for downstream distributions to cherry-pick a minimal patch from upstream.

Niklas

On 2018-12-25 21:29, Florian Weimer wrote:
> This patch removes the custom memcpy implementation from _int_realloc
> for small chunk sizes.  ncopies has the wrong type and could thus
> result in too few elements being copied, so removing this code fixes
> bug 24027.  I don't think the inlining is performance-critical because
> this code is used when the realloc results in an _int_malloc, copy,
> and _int_free, so even for small allocations there is quote some
> overhead beyond the copy itself.
> 
> I'm not sure if this warrants tracking as a security bug.  Looking at
> the code, the problem could be trigger in a default configuration if
> mremap fails and a subsequent mmap succeeds.
Siddhesh Poyarekar Dec. 27, 2018, 1:47 a.m. | #2
On 26/12/18 1:59 AM, Florian Weimer wrote:
> This patch removes the custom memcpy implementation from _int_realloc
> for small chunk sizes.  ncopies has the wrong type and could thus
> result in too few elements being copied, so removing this code fixes
> bug 24027.  I don't think the inlining is performance-critical because
> this code is used when the realloc results in an _int_malloc, copy,
> and _int_free, so even for small allocations there is quote some
> overhead beyond the copy itself.
> 
> I'm not sure if this warrants tracking as a security bug.  Looking at
> the code, the problem could be trigger in a default configuration if
> mremap fails and a subsequent mmap succeeds.
> 
> 2018-12-23  Florian Weimer  <fw@deneb.enyo.de>
> 
> 	[BZ #24027]
> 	* malloc/malloc.c (_int_realloc): Always call memcpy for the
> 	copying operation.
> 

Fix looks OK to me.  Please add an explicit note in the description 
about the integer overflow so that if anybody tries to revert this patch 
in future they know that there's more to fix in there.

Siddhesh
Siddhesh Poyarekar Dec. 27, 2018, 1:55 a.m. | #3
On 26/12/18 2:25 AM, Niklas Hamb├╝chen wrote:
> Would it not make sense to first apply the patch I provided in
> 
>    https://sourceware.org/bugzilla/show_bug.cgi?id=24027
> 
> that fixes the type, and then do the removal of the memcpy special case as a refactoring?
> 
> That would ensure that if somebody disagrees with the performance-critical bit in the future and reverts this, they don't revert back to the bugged version.
> It would also make it possible for downstream distributions to cherry-pick a minimal patch from upstream.

I agree with your point about reverting buggy behaviour and I've 
suggested adding a note in the commit to guard against that.

It is Florian's discretion if he wants to split this into two commits 
like you suggest; it's not strictly necessary IMO (performance impact 
and revert risk are negligible and Florian's patch is also minimal 
enough for most distros) but is not that big a deal to do either.

Thanks,
Siddhesh

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index c33709e966..0a20a6054c 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4581,11 +4581,6 @@  _int_realloc(mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize,
   mchunkptr        remainder;       /* extra space at end of newp */
   unsigned long    remainder_size;  /* its size */
 
-  unsigned long    copysize;        /* bytes to copy */
-  unsigned int     ncopies;         /* INTERNAL_SIZE_T words to copy */
-  INTERNAL_SIZE_T* s;               /* copy source */
-  INTERNAL_SIZE_T* d;               /* copy destination */
-
   /* oldmem size */
   if (__builtin_expect (chunksize_nomask (oldp) <= 2 * SIZE_SZ, 0)
       || __builtin_expect (oldsize >= av->system_mem, 0))
@@ -4653,43 +4648,7 @@  _int_realloc(mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize,
             }
           else
             {
-              /*
-                 Unroll copy of <= 36 bytes (72 if 8byte sizes)
-                 We know that contents have an odd number of
-                 INTERNAL_SIZE_T-sized words; minimally 3.
-               */
-
-              copysize = oldsize - SIZE_SZ;
-              s = (INTERNAL_SIZE_T *) (chunk2mem (oldp));
-              d = (INTERNAL_SIZE_T *) (newmem);
-              ncopies = copysize / sizeof (INTERNAL_SIZE_T);
-              assert (ncopies >= 3);
-
-              if (ncopies > 9)
-                memcpy (d, s, copysize);
-
-              else
-                {
-                  *(d + 0) = *(s + 0);
-                  *(d + 1) = *(s + 1);
-                  *(d + 2) = *(s + 2);
-                  if (ncopies > 4)
-                    {
-                      *(d + 3) = *(s + 3);
-                      *(d + 4) = *(s + 4);
-                      if (ncopies > 6)
-                        {
-                          *(d + 5) = *(s + 5);
-                          *(d + 6) = *(s + 6);
-                          if (ncopies > 8)
-                            {
-                              *(d + 7) = *(s + 7);
-                              *(d + 8) = *(s + 8);
-                            }
-                        }
-                    }
-                }
-
+	      memcpy (newmem, chunk2mem (oldp), oldsize - SIZE_SZ);
               _int_free (av, oldp, 1);
               check_inuse_chunk (av, newp);
               return chunk2mem (newp);