X-Authentication-Warning:	delorie.com: mail set sender to djgpp-workers-bounces using -f
X-Recipient:	djgpp-workers AT delorie DOT com
X-Original-DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed;
	d=gmail.com; s=20230601; t=1709032303; x=1709637103; darn=delorie.com;
	h=content-transfer-encoding:to:subject:message-id:date:from
	:in-reply-to:references:mime-version:from:to:cc:subject:date
	:message-id:reply-to;
	bh=zxF0oDrITCGNXVkM0rweEFoo7O7RG5ztht+GFq2KB1g=;
	b=CVWyEAflWzrPUfqWXrwCAA/3Dd2vIsd5n0uNu1KiC9IyD4l/GdOpwBjOkGXNTqL9CH
	zFo1DGXl2BWuSXPA6JGK/FSFARkbsp5U3iHiYjuN/bqIIT40IhbZVB3AAEwUiPT8Orpd
	Tm8Le4qbF6BI+/1psqUEDzLjrFNQVCXIi6QZmFUs9GRpxa60MoZpXM0/qp9eTcPAviHm
	nRW7UYTXI+RJVkSc/lZrPOlvGmFzdfdY0dT5uH7oS2K6mKThiq0ijQYOWx4fusV8yvk1
	8z1krDrlVGxuDFF0n2MtHRcB/ym5BlxktBZbA8iMVLY9A2uIoXBawvm6zn/4czu5oNoh
	bxdw==
X-Google-DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed;
	d=1e100.net; s=20230601; t=1709032303; x=1709637103;
	h=content-transfer-encoding:to:subject:message-id:date:from
	:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc
	:subject:date:message-id:reply-to;
	bh=zxF0oDrITCGNXVkM0rweEFoo7O7RG5ztht+GFq2KB1g=;
	b=TLmHyhXbzZwFSuFFJbOoerFoOrkM1TDc9zb3c7dOZMwg72wwx8ZTcafbDF0jCPsXA/
	no4UEEX3YLNZe5pf2EK4KcAc1DvQXi6r23bHh5rp6IIrAORpzQQXuXqCI/JA8ukrfLth
	Hcvh4S0ILzBjmxNtdaHdUvdzzCX32E80fi/4W/UyUytryyPUn8bcswfmVo5MurN13OwD
	GOD2SWWJh4mdo90aYQNY3yfjybvosfeKnIdzGxo6LU5dAd0RjAR+Td7DIrcqVhNNXqE1
	nQ47qh4bZN5i3/t+XADToDJ0jpQLQSzHITvZRywG6/DhVJZ9UYKh4nQAu42tzuMVcaYf
	rcsg==
X-Gm-Message-State:	AOJu0YwEnT9C/EMv5PImSImalfMIhGoUE9R2vA6Ft80SrEkN9HmbA7LS
	KxxNedqqP6NUwBJCyl0qANj7ZwaHQUayvTvlr/MCW3Ye0wQ6IPZyXa5fUeiNdRB04cd0I9xQPE/
	EAlGyA7ORTxOaWzGi5KS0wqDvtk8Ew3tM
X-Google-Smtp-Source:	AGHT+IEEByqBQq9yBZD8hGrXB1GIcW1n2Ojl7suwCStgupNqpLSlp0sSqkbdTWbRQg0MvAig3ZNqgtnrW1h3R78D/7U=
X-Received:	by 2002:a5d:6d85:0:b0:33d:dd41:6b81 with SMTP id
	l5-20020a5d6d85000000b0033ddd416b81mr3767869wrs.60.1709032302910; Tue, 27 Feb
	2024 03:11:42 -0800 (PST)
MIME-Version:	1.0
References:	<20181121121721 DOT GA1501 AT b21e7fa156c1e52f6cc3a9e0ddf22975>
	<CAA2C=vC30DQkZ95EESRDMVVXfML_741-V=zKCHtfRXfYM1tXgQ AT mail DOT gmail DOT com> <CAGFXeQJdeV97F5JfZw+ij5WkcasNHG8+7gTqYnQ3D6=RDYpeDg AT mail DOT gmail DOT com>
In-Reply-To:	<CAGFXeQJdeV97F5JfZw+ij5WkcasNHG8+7gTqYnQ3D6=RDYpeDg@mail.gmail.com>
From:	"Ozkan Sezer (sezeroz AT gmail DOT com) [via djgpp-workers AT delorie DOT com]" <djgpp-workers AT delorie DOT com>
Date:	Tue, 27 Feb 2024 14:11:31 +0300
Message-ID:	<CAA2C=vBSnNnAxkSvnZkNj-t31Vj5piYF0__3EtiOfrfAhZg65Q@mail.gmail.com>
Subject:	Re: memalign broken (again)
To:	djgpp-workers AT delorie DOT com
X-MIME-Autoconverted:	from quoted-printable to 8bit by delorie.com id 41RBBjWV875299
Reply-To:	djgpp-workers AT delorie DOT com

On Tue, Feb 27, 2024 at 8:34 AM Daniel Verkamp (daniel AT drv DOT nu) [via
djgpp-workers AT delorie DOT com] <djgpp-workers AT delorie DOT com> wrote:
>
> 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.

Finally, some progress with this

> 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 */

Can anyone review this?

- Raw text -