malloc: Always call memcpy in _int_realloc [BZ #24027]
Commit Message
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
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.
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
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
@@ -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);