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> In-Reply-To: From: "Ozkan Sezer (sezeroz AT gmail DOT com) [via djgpp-workers AT delorie DOT com]" Date: Tue, 27 Feb 2024 14:11:31 +0300 Message-ID: Subject: Re: memalign broken (again) To: djgpp-workers AT delorie DOT com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit 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] wrote: > > On Wed, Nov 21, 2018 at 4:33 AM Ozkan Sezer (sezeroz AT gmail DOT com) [via > djgpp-workers AT delorie DOT com] wrote: > > > > On 11/21/18, Peter Ross 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 > > > #include > > > > > > #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?