Re: [PATCH v2] malloc:New test to check malloc alternate path using memory obstruction

["Zack Weinberg" <zack@owlfolio.org>]

On Tue, May 21, 2024, at 11:10 AM, sayan paul wrote:
> The test aims to ensure that malloc uses the alternate path to
> allocate memory when sbrk() or brk() fails.To achieve this,
> the test first creates an obstruction at current program break,
> tests that obstruction with a failing sbrk(),then checks if malloc
> is still returning a valid ptr thus inferring that malloc() used
> mmap() instead of brk() or sbrk() to allocate the memory.

This is definitely an improvement on the first version you sent.
Now that it is easier to understand what the test does, I have a
few more suggestions.

> @@ -408,3 +409,4 @@ tst-mallocstate-malloc-check-ENV = 
> LD_PRELOAD=$(objpfx)libc_malloc_debug.so
>  # libc_malloc_debug.so.
>  $(objpfx)tst-mallocstate: $(objpfx)libc_malloc_debug.so
>  $(objpfx)tst-mallocstate-malloc-check: $(objpfx)libc_malloc_debug.so
> +

This addition of a blank line at the end of the Makefile looks like
an editing mistake, please remove it again.

> --- /dev/null
> +++ b/malloc/tst-malloc-alternate-path.c
> @@ -0,0 +1,100 @@
> +/* Test that malloc uses mmap when sbrk or brk fails.
> +   This code returns success when there is an obstruction setup
> +   and sbrk() fails to grow the heap size forcing malloc to use mmap().
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

Very minor, but the explanation on line one of what the file does
is supposed to be *exactly one line*.  Move the second sentence to a
separate comment below the copyright boilerplate, please.  This might
not be just cosmetic; I don't know if the script for updating copyright
years does the right thing if the 'Copyright (C)' isn't on line two.

...
> +  /* Get current program break */
...
> +  /* Get the runtime page size */
...
> +  /* Round up to the next page boundary */
...
> +  /* Place a mapping using mmap at the next page boundary */
> +  void *obstruction_addr
> +      = mmap (next_page_boundary, page_size, PROT_WRITE | PROT_READ,
> +	      MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);

Suggest using PROT_NONE, or PROT_READ alone, here, so that a malloc
implementation that tries to write into the obstruction will crash.

> +  if (obstruction_addr == MAP_FAILED)
> +    {
> +      perror ("mmap");
> +      return 1;

Isn't there a special exit code to distinguish "the test itself
malfunctioned" from "the test has failed"?  Returning 99 instead of 1
or something like that?

> +  /* Try to extend the heap beyond the obstruction using sbrk */
> +  int *ptr = (int *) sbrk (sizeof (int));
> +  if (ptr != (void *) -1)

Okay, here's the main thing I wanted to point out.  Did you know that
the break address is *not* necessarily aligned to a page boundary?
The kernel doesn't enforce any alignment at all, in fact.  You
correctly handle this possibility earlier in the code, rounding the
value returned by sbrk(0) up to the nearest page to use as the address
for the mmap obstruction.  But here, if the break address happened to
be in the middle of a page, the attempt to advance it by sizeof(int)
would probably *succeed* despite the obstruction at the next page
boundary.

Our malloc always asks sbrk() for a large chunk of memory that's a
multiple of the page size, so it won't come up as long as this test is
only used to test our malloc.  But, alternative malloc implementations
might move the break in small increments, and our malloc *testsuite*
should, as much as possible, be usable with alternative implementations.
This is both because we might change out our malloc implementation
someday, and because people *writing* malloc implementations might
like to steal our testsuite.

So, what you should be doing here is allocating an entire page:

    void *ptr = sbrk (page_size)

That will reliably hit the obstruction.

> +    {
> +      fprintf (stderr, "memory allocation can be done using sbrk.\n");
> +      free (ptr);
> +      return 1;
> +    }

ptr here was received directly from sbrk(), so passing it to free() is
incorrect.  The next line exits the process, so there's no need to
avoid a memory leak - just drop the free.

> +  /* Check if malloc changed program break */
> +  if (current_brk != new_brk)
> +    {
> +      fprintf (stderr, "malloc changed program break\n");
> +      free (memptr);
> +      return 1;
> +    }
> +  free (memptr);
> +
> +  /* Free the obstruction mapping */
> +  if (munmap (obstruction_addr, page_size) == -1)
> +    {
> +      perror ("munmap");
> +    }
> +
> +  return 0;
> +}

Similarly, I would just have

    if (current_brk != new_brk)
      {
        fputs ("malloc changed program break\n", stderr);
        return 1;
      }
    return 0;
  }

as the last few lines.

Finally, I think it would be a a good idea to test doing a bunch of
*small* allocations with malloc, as well as the one allocation of
LARGE_SIZE that you do have.  It's possible that the test is
succeeding only because LARGE_SIZE is large enough to be allocated
with mmap regardless.  Something like

  for (size_t i = 0; i < page_size / alignof (max_align_t); i++)
    {
      // intentional memory leak on next line
      if (! malloc (alignof (max_align_t)))
        {
          perror ("malloc [small]");
          return 1;
        }
    }

This should be first, and the large allocation second.  That way, if
the test fails spuriously because someone set the memory rlimits too
low, it will be easier to figure out that that's the problem.

zw