| Message ID | 87sgyluz8t.fsf@mid.deneb.enyo.de |
|---|---|
| State | New |
| Headers | show |
| Series | malloc: Always call memcpy in _int_realloc [BZ #24027] | expand |
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
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);