Mail Archives: djgpp-workers/2024/02/27/00:34:24
On Wed, Nov 21, 2018 at 4:33 AM Ozkan Sezer (sezeroz AT gmail DOT com) [via
djgpp-workers AT delorie DOT com] <djgpp-workers AT delorie DOT com> wrote:
>
> On 11/21/18, Peter Ross <pross AT xvid DOT org> wrote:
> > hi,
> >
> > djgpp memalign still fails when:
> >
> > 1. alignment is >= 16 bytes
> > 2. some sequence of memory allocations have already occured.
> >
> > the test case below is reproducible with djgpp cvs and 2.05+nmemalign.patch
> > (patch was found here:
> > https://aur.archlinux.org/cgit/aur.git/tree/nmemalign.patch?h=dosbox-djcrx)
> >
> > tested using freedos 1.2 on physical hardware.
> >
> > ---
> > #include <stdio.h>
> > #include <malloc.h>
> >
> > #define TEST(alignment, size) \
> > printf("memalign(%d,%d)=", alignment, size); \
> > ptr = memalign(alignment, size); \
> > printf("%p\n", ptr);
> >
> > int main()
> > {
> > void * ptr;
> > TEST(1, 46)
> > TEST(4, 583)
> > TEST(64, 773)
> > TEST(32, 889) /* <-- reliably fails here */
> > return 0;
> > }
>
> Did a quick build & run, reproduces under dosemu and dosbox, too.
Apologies for reviving an old thread, but I think I found the root
cause of this bug.
The problem is in the branch where the allocated block is not
pre-aligned to the desired alignment, which means the block needs to
be split into two parts and the first one freed. The size passed to
__nmalloc_split(), which is supposed to include the space for the
memblock header plus the allocated data region, is calculated as
`alignment - misalign`, which is always too small. For example, if the
requested alignment is 16 bytes, the calculated size for the split
will be 15 bytes, which is too small to contain a memblock structure
(20+ bytes). This will result in a corrupted free list when the
newly-created memblock is written on top of part of the original one.
The size passed to nmalloc() includes extra space (3 * ALIGN in the
XTRA definition) to ensure that there is room to insert another
memblock header, but the split calculation never uses it.
One possible fix (patch below) is to calculate the region to be
returned by adding XTRA (which includes the alignment plus the minimal
space needed for a new memblock), adjusting the resulting pointer down
so that it will be a multiple of the requested alignment (this will
always be within the bounds of the allocation due to the XTRA
addition), then using the MEMBLKp macro to calculate the appropriate
position for the new memblock to be created by the split, and finally
using the difference between the original and aligned memblocks as the
size for the __nmalloc_split() call. This ensures that the returned
pointer is always actually aligned to the requested value, which I
don't think the old code would have done even assuming it did not
corrupt the free list - the `misalign` calculation is based on the
original data location, but the split would adjust this pointer by
DATAOFFSET when adding the new memblock, so the resulting split block
would have always been misaligned.
---
src/libc/ansi/stdlib/nmalign.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/libc/ansi/stdlib/nmalign.c b/src/libc/ansi/stdlib/nmalign.c
index 541d1a8b..90ee4c4b 100644
--- a/src/libc/ansi/stdlib/nmalign.c
+++ b/src/libc/ansi/stdlib/nmalign.c
@@ -110,8 +110,9 @@ static inline int invalid(size_t alignment)
void *nmemalign(size_t alignment, size_t size)
{
- memblockp m = NULL;
- void *minit;
+ memblockp m = NULL, m1;
+ char *minit;
+ char *malign;
ulong misalign;
size_t szneed, sz = size; /* preserve arg for hooks */
@@ -141,7 +142,9 @@ void *nmemalign(size_t alignment, size_t size)
/* two or more chunks to release */
DBGPRTM(" Complex case, release multiple chunks");
DBGEOLN;
- nfree(PTR(__nmalloc_split(&m, alignment - misalign)));
+ malign = (char *)(((ulong)minit + XTRA - 1) & ~(alignment - 1));
+ m1 = MEMBLKp(malign);
+ nfree(PTR(__nmalloc_split(&m, (ulong)m1 - (ulong)m)));
return nrealloc(PTR(m), size);
}
} /* alignment > ALIGN */
--
- Raw text -