[Qemu-devel] [PATCH v2] linux-user: Fix the computation of the requested heap size

[Cédric VINCENT]

There were several remaining bugs in the previous implementation of
do_brk():

    1. the value of "new_alloc_size" was one page too large when the
       requested brk was aligned on a host page boundary.

    2. no new pages should be (re-)allocated when the requested brk is
       in the range of the pages that were already allocated
       previsouly (for the same purpose).  Technically these pages are
       never unmapped in the current implementation.

The problem/fix can be reproduced/validated with the test-suite above:

    #include <unistd.h>       /* syscall(2),      */
    #include <sys/syscall.h>  /* SYS_brk,         */
    #include <stdio.h>        /* puts(3),         */
    #include <stdlib.h>       /* exit(3), EXIT_*, */
    #include <stdint.h>       /* uint*_t,         */
    #include <sys/mman.h>     /* mmap(2), MAP_*,  */
    #include <string.h>       /* memset(3), */

    int main()
    {
        int exit_status = EXIT_SUCCESS;
        uint8_t *current_brk = 0;
        uint8_t *initial_brk;
        uint8_t *new_brk;
        uint8_t *old_brk;
        int failure = 0;
        int i;

        void test_brk(int increment, int expected_result) {
            new_brk = (uint8_t *)syscall(SYS_brk, current_brk + increment);
            if ((new_brk == current_brk) == expected_result)
                failure = 1;
            current_brk = (uint8_t *)syscall(SYS_brk, 0);
        }

        void test_result() {
            if (!failure)
                puts("OK");
            else {
                puts("failure");
                exit_status = EXIT_FAILURE;
            }
        }

        void test_title(const char *title) {
            failure = 0;
            printf("%-45s : ", title);
            fflush(stdout);
        }

        test_title("Initialization");
        test_brk(0, 1);
        initial_brk = current_brk;
        test_result();

        test_title("Don't overlap \"brk\" pages");
        test_brk(HOST_PAGE_SIZE, 1);
        test_brk(HOST_PAGE_SIZE, 1);
        test_result();

        /* Preparation for the test "Re-allocated heap is initialized".  */
        old_brk = current_brk - HOST_PAGE_SIZE;
        memset(old_brk, 0xFF, HOST_PAGE_SIZE);

        test_title("Don't allocate the same \"brk\" page twice");
        test_brk(-HOST_PAGE_SIZE, 1);
        test_brk(HOST_PAGE_SIZE, 1);
        test_result();

        test_title("Re-allocated \"brk\" pages are initialized");
        for (i = 0; i < HOST_PAGE_SIZE; i++) {
            if (old_brk[i] != 0) {
                printf("(index = %d, value = 0x%x) ", i, old_brk[i]);
                failure = 1;
                break;
            }
        }
        test_result();

        test_title("Don't allocate \"brk\" pages over \"mmap\" pages");
        new_brk = mmap(current_brk, HOST_PAGE_SIZE / 2, PROT_READ, MAP_PRIVATE 
| MAP_ANONYMOUS | MAP_FIXED, -1, 0);
        if (new_brk == (void *) -1)
            puts("unknown");
        else {
            test_brk(HOST_PAGE_SIZE, 0);
            test_result();
        }

        test_title("All \"brk\" pages are writable (please wait)");
        if (munmap(current_brk, HOST_PAGE_SIZE / 2) != 0)
            puts("unknown");
        else {
            while (current_brk - initial_brk < 2*1024*1024*1024UL) {
                old_brk = current_brk;

                test_brk(HOST_PAGE_SIZE, -1);
                if (old_brk == current_brk)
                    break;

                for (i = 0; i < HOST_PAGE_SIZE; i++)
                    old_brk[i] = 0xAA;
            }
            puts("OK");
        }

        test_title("Maximum size of the heap > 16MB");
        failure = (current_brk - initial_brk) < 16*1024*1024;
        test_result();

        exit(exit_status);
    }

Changes introduced in patch v2:

    * extend the "brk" test-suite embedded within the commit message;

    * heap contents have to be initialized to zero, this bug was
      exposed by "tst-calloc.c" from the GNU C library;

    * don't [try to] allocate a new host page if the new "brk" is
      equal to the latest allocated host page ("brk_page"); and

    * print some debug information when DEBUGF_BRK is defined.

Signed-off-by: Cédric VINCENT <address@hidden>
Reviewed-by: Christophe Guillon <address@hidden>
Cc: Riku Voipio <address@hidden>
---
 linux-user/syscall.c |   37 +++++++++++++++++++++++++++++--------
 1 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b975730..608924a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -709,29 +709,44 @@ char *target_strerror(int err)
 
 static abi_ulong target_brk;
 static abi_ulong target_original_brk;
+static abi_ulong brk_page;
 
 void target_set_brk(abi_ulong new_brk)
 {
     target_original_brk = target_brk = HOST_PAGE_ALIGN(new_brk);
+    brk_page = HOST_PAGE_ALIGN(target_brk);
 }
 
+//#define DEBUGF_BRK(message, args...) do { fprintf(stderr, (message), ## 
args); } while (0)
+#define DEBUGF_BRK(message, args...)
+
 /* do_brk() must return target values and target errnos. */
 abi_long do_brk(abi_ulong new_brk)
 {
-    abi_ulong brk_page;
     abi_long mapped_addr;
     int        new_alloc_size;
 
-    if (!new_brk)
+    DEBUGF_BRK("do_brk(%#010x) -> ", new_brk);
+
+    if (!new_brk) {
+        DEBUGF_BRK("%#010x (!new_brk)\n", target_brk);
         return target_brk;
-    if (new_brk < target_original_brk)
+    }
+    if (new_brk < target_original_brk) {
+        DEBUGF_BRK("%#010x (new_brk < target_original_brk)\n", target_brk);
         return target_brk;
+    }
 
-    brk_page = HOST_PAGE_ALIGN(target_brk);
-
-    /* If the new brk is less than this, set it and we're done... */
-    if (new_brk < brk_page) {
+    /* If the new brk is less than the highest page reserved to the
+     * target heap allocation, set it and we're almost done...  */
+    if (new_brk <= brk_page) {
+        /* Heap contents are initialized to zero, as for anonymous
+         * mapped pages.  */
+        if (new_brk > target_brk) {
+            memset(g2h(target_brk), 0, new_brk - target_brk);
+        }
        target_brk = new_brk;
+        DEBUGF_BRK("%#010x (new_brk <= brk_page)\n", target_brk);
        return target_brk;
     }
 
@@ -741,13 +756,15 @@ abi_long do_brk(abi_ulong new_brk)
      * itself); instead we treat "mapped but at wrong address" as
      * a failure and unmap again.
      */
-    new_alloc_size = HOST_PAGE_ALIGN(new_brk - brk_page + 1);
+    new_alloc_size = HOST_PAGE_ALIGN(new_brk - brk_page);
     mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
                                         PROT_READ|PROT_WRITE,
                                         MAP_ANON|MAP_PRIVATE, 0, 0));
 
     if (mapped_addr == brk_page) {
         target_brk = new_brk;
+        brk_page = HOST_PAGE_ALIGN(target_brk);
+        DEBUGF_BRK("%#010x (mapped_addr == brk_page)\n", target_brk);
         return target_brk;
     } else if (mapped_addr != -1) {
         /* Mapped but at wrong address, meaning there wasn't actually
@@ -755,6 +772,10 @@ abi_long do_brk(abi_ulong new_brk)
          */
         target_munmap(mapped_addr, new_alloc_size);
         mapped_addr = -1;
+        DEBUGF_BRK("%#010x (mapped_addr != -1)\n", target_brk);
+    }
+    else {
+        DEBUGF_BRK("%#010x (otherwise)\n", target_brk);
     }
 
 #if defined(TARGET_ALPHA)
-- 
1.7.5.1