bitwiseworks / libcx

kLIBC Extension Library
GNU Lesser General Public License v2.1
11 stars 1 forks source link

free_mmap will hang in the loop at line 1288 for some memory layouts. #101

Closed StevenLevine closed 2 years ago

StevenLevine commented 2 years ago

https://github.com/bitwiseworks/libcx/commit/09b0eaa459fe7b3f2c61eb670d8f56c8c0a29ad2

introduced logic to find and free allocations that are no longer in use by mmap. For certain mmap/munmap patterns the loop at line 1288 will hang:

src\mmap\mmap.c:1288 ULONG start = m->start & 0xFFFF0000; if (start == m->start) start -= 0x10000; while (start) { len = ~0; arc = DosQueryMem((PVOID)start, &len, &mem_flags); ASSERT_MSG(arc == NO_ERROR, "%ld", arc); if (mem_flags & PAG_BASE) break; }

This happens when the first address checked by DosQueryMem is not a PAG_BASE. This occurs because the pointer is not decremented inside the loop. The code needs to be:

    ULONG start = m->start & 0xFFFF0000; /* Align to 64K boundary */
    ASSERT_MSG(start != NULL, "start == NULL");
    for (;;)
    {
      len = ~0;
      arc = DosQueryMem((PVOID)start, &len, &mem_flags);
      ASSERT_MSG(arc == NO_ERROR, "%ld", arc);
      if (mem_flags & PAG_BASE)
        break;
      start -= 0x10000;
    }

This logic assumes that the address marked PAG_BASE was originally allocated by mmap. The existing asserts should ensure this.

If you would like a pull request for this change, let me know.

I have a testcase that reliably demonstrates the hang.

The original code that triggered this failure mode was in the same php port that produced #71 and a few other tickets, but it is no longer present. It was part of the code that the php interpreter's memory manager uses to allocate 2MB aligned chunks of mmap'ed memory As mentioned in #76, the partial unmap cannot free the memory for other uses by the OS/2 kernel. The result was that almost every 2MB chunk used 4MB of address and on a busy httpd/php server this was a recipe for disaster. This code has been rewritten so that the aligned 2MB chunks only use 2MB of address space.

The initial commit for this rework is at https://github.com/StevenLevine/php-7-os2/commit/4c59cef157d3a6502e8dac2751928c935e4170f2

dmik commented 2 years ago

Steve, thanks for shedding light on this place. I must had been sleeping when committing or such. Obviously, the decrement should be in the loop. BTW, asserting that start != NULL isn't needed if you use while as the original code does — it's asserted after the loop anyway.

dmik commented 2 years ago

Ah and please test my version of the commit with your test case!

StevenLevine commented 2 years ago

I'm not seeing an updated libcx rpm in netlabs-exp. Should I? Thanks.

dmik commented 2 years ago

It's awaiting more fixes.