Message ID | xnr22tevec.fsf@greed.delorie.com |
---|---|
State | New |
Headers | show |
Series | [v1] malloc: fix set_max_fast "impossibly small" value | expand |
On 10/30/19 6:11 PM, DJ Delorie wrote: > > From 83035919da9208a7e6666bdde60e110e2e301553 Mon Sep 17 00:00:00 2001 > From: DJ Delorie <dj@redhat.com> > Date: Wed, 30 Oct 2019 18:03:14 -0400 > Subject: Base max_fast on alignment, not width, of bins Does this fix bug 24903? If it does please add "(Bug 24903)" to the first line of your git commit. I assume this message is your git format-patch HEAD~1 output. OK for master with the following fixed: - Add "(Bug 24903)" to the git commit first line. - Apply both commit message changes. > set_max_fast set the "impossibly small" value based on, s/set/sets/g > eventually, MALLOC_ALIGNMENT. The comparisons for the smallest > chunk used, eventuall, MIN_CHUNK_SIZE. Note that i386 s/, eventuall/is, eventually/g > is the only platform where these are the same, so a smallest > chunk *would* be put in a no-fastbins fastbin. > > This change calculates the "impossibly small" value > based on MIN_CHUNK_SIZE instead, so that we can know it will > always be impossibly small. > > diff --git a/malloc/malloc.c b/malloc/malloc.c > index 5d3e82a8f6..70cc35a473 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -1621,7 +1621,7 @@ static INTERNAL_SIZE_T global_max_fast; > > #define set_max_fast(s) \ > global_max_fast = (((s) == 0) \ > - ? SMALLBIN_WIDTH : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK)) > + ? MIN_CHUNK_SIZE / 2 : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK)) OK. My analysis: SMALLBIN_WIDTH is MALLOC_ALIGNMENT: 1403 #define SMALLBIN_WIDTH MALLOC_ALIGNMENT Which on i386 is overriden here: sysdeps/i386/malloc-alignment.h: 22 #define MALLOC_ALIGNMENT 16 This is because we have to account for 16-byte aligned vector types. This means SMALLBIN_WIDTH is 16, which for 32-bit targets is an aligned and perfectly viable chunk size. 1178 /* The smallest possible chunk */ 1179 #define MIN_CHUNK_SIZE (offsetof(struct malloc_chunk, fd_nextsize)) This is because MIN_CHUNK_SIZE is: 1048 struct malloc_chunk { 1049 1050 INTERNAL_SIZE_T mchunk_prev_size; /* Size of previous chunk (if free). */ 1051 INTERNAL_SIZE_T mchunk_size; /* Size in bytes, including overhead. */ 1052 1053 struct malloc_chunk* fd; /* double links -- used only if free. */ 1054 struct malloc_chunk* bk; 1055 1056 /* Only used for large blocks: pointer to next larger size. */ 1057 struct malloc_chunk* fd_nextsize; /* double links -- used only if free. */ 1058 struct malloc_chunk* bk_nextsize; 1059 }; The offsetof returns 16-bytes. On x86_64 we have SMALLBIN_WIDTH of 16, but the MIN_CHUNK_SIZE is 32, so it works. It's the increase in MALLOC_ALIGNMENT to 16 for i686 which breaks this. Your solution of MIN_CHUNK_SIZE/2 works. On i686 it yields 8, a size that is too small. On x86_64 it yields 16, a size that is too small. Thus it looks like the new calculation in set_max_fast yields an impossibly small value that we can use that no result of request2size() will return. > > static inline INTERNAL_SIZE_T > get_max_fast (void) >
On Wed, 30 Oct 2019, Carlos O'Donell wrote: > Which on i386 is overriden here: > sysdeps/i386/malloc-alignment.h: > 22 #define MALLOC_ALIGNMENT 16 > > This is because we have to account for 16-byte aligned vector types. Rather, to account for 16-byte-aligned _Float128 and _Decimal128. Vector types are expected to require aligned_alloc, but any type that's a basic type as defined in ISO C has a fundamental alignment and so malloc must allocate memory suitably aligned for it.
On 10/31/19 12:56 PM, Joseph Myers wrote: > On Wed, 30 Oct 2019, Carlos O'Donell wrote: > >> Which on i386 is overriden here: >> sysdeps/i386/malloc-alignment.h: >> 22 #define MALLOC_ALIGNMENT 16 >> >> This is because we have to account for 16-byte aligned vector types. > > Rather, to account for 16-byte-aligned _Float128 and _Decimal128. Vector > types are expected to require aligned_alloc, but any type that's a basic > type as defined in ISO C has a fundamental alignment and so malloc must > allocate memory suitably aligned for it. Thanks for the clarification.
diff --git a/malloc/malloc.c b/malloc/malloc.c index 5d3e82a8f6..70cc35a473 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -1621,7 +1621,7 @@ static INTERNAL_SIZE_T global_max_fast; #define set_max_fast(s) \ global_max_fast = (((s) == 0) \ - ? SMALLBIN_WIDTH : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK)) + ? MIN_CHUNK_SIZE / 2 : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK)) static inline INTERNAL_SIZE_T get_max_fast (void)
From 83035919da9208a7e6666bdde60e110e2e301553 Mon Sep 17 00:00:00 2001 From: DJ Delorie <dj@redhat.com> Date: Wed, 30 Oct 2019 18:03:14 -0400 Subject: Base max_fast on alignment, not width, of bins set_max_fast set the "impossibly small" value based on, eventually, MALLOC_ALIGNMENT. The comparisons for the smallest chunk used, eventuall, MIN_CHUNK_SIZE. Note that i386 is the only platform where these are the same, so a smallest chunk *would* be put in a no-fastbins fastbin. This change calculates the "impossibly small" value based on MIN_CHUNK_SIZE instead, so that we can know it will always be impossibly small.