[Qemu-devel] [PATCH 0/3] Fix ARM semihosting SYS_HEAPINFO issues

[Qemu-devel] [PATCH 0/3] Fix ARM semihosting SYS_HEAPINFO issues

[Peter Maydell]

This patchset is intended to fix some problems with the ARM
semihosting SYS_HEAPINFO call. Patch 1 fixes a bug in do_brk()
which meant that using SYS_HEAPINFO tended to result in our
accidentally unmapping the host libc. Patch 2 fixes the
bug https://bugs.launchpad.net/qemu/+bug/656285
by correcting the check for failure of do_brk(). Patch 3
does the same for the equivalent code in m68k-semi.c, but
note that I have only tested that it compiles.

(linux-user/m68k-sim.c also has a suspicious error check
on do_brk(), but since I don't have any specs of what the
simcalls there are supposed to do on error I haven't
attempted to fix this one.)

Peter Maydell (3):
  linux-user: Don't use MAP_FIXED in do_brk()
  arm-semi.c: Use correct check for failure of do_brk()
  m68k-semi.c: Use correct check for failure of do_brk()

 arm-semi.c           |    5 +++--
 linux-user/syscall.c |   29 ++++++++++++++++++++---------
 m68k-semi.c          |    5 +++--
 3 files changed, 26 insertions(+), 13 deletions(-)

[Qemu-devel] [PATCH 1/3] linux-user: Don't use MAP_FIXED in do_brk()

[Peter Maydell]

Since mmap() with MAP_FIXED will map over the top of existing mappings,
it's a bad idea to use it to implement brk(), because brk() with a
large size is likely to overwrite important things like qemu itself
or the host libc. So we drop MAP_FIXED and handle "mapped but at
different address" as an error case instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index bb0999d..e68c5e0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -733,23 +733,34 @@ abi_long do_brk(abi_ulong new_brk)
     	return target_brk;
     }
 
-    /* We need to allocate more memory after the brk... */
+    /* We need to allocate more memory after the brk... Note that
+     * we don't use MAP_FIXED because that will map over the top of
+     * any existing mapping (like the one with the host libc or qemu
+     * 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);
     mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
                                         PROT_READ|PROT_WRITE,
-                                        MAP_ANON|MAP_FIXED|MAP_PRIVATE, 0, 0));
+                                        MAP_ANON|MAP_PRIVATE, 0, 0));
+
+    if (mapped_addr == brk_page) {
+        target_brk = new_brk;
+        return target_brk;
+    } else if (mapped_addr != -1) {
+        /* Mapped but at wrong address, meaning there wasn't actually
+         * enough space for this brk.
+         */
+        target_munmap(mapped_addr, new_alloc_size);
+        mapped_addr = -1;
+    }
 
 #if defined(TARGET_ALPHA)
     /* We (partially) emulate OSF/1 on Alpha, which requires we
        return a proper errno, not an unchanged brk value.  */
-    if (is_error(mapped_addr)) {
-        return -TARGET_ENOMEM;
-    }
+    return -TARGET_ENOMEM;
 #endif
-
-    if (!is_error(mapped_addr)) {
-	target_brk = new_brk;
-    }
+    /* For everything else, return the previous break. */
     return target_brk;
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/3] linux-user: Don't use MAP_FIXED in do_brk()

[Peter Maydell]

On 18 April 2011 16:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> Since mmap() with MAP_FIXED will map over the top of existing mappings,
> it's a bad idea to use it to implement brk(), because brk() with a
> large size is likely to overwrite important things like qemu itself
> or the host libc. So we drop MAP_FIXED and handle "mapped but at
> different address" as an error case instead.

I've had a report from Martin Mohring that this patch breaks some
programs which previously worked in linux-user mode (one wonders if
they were overwriting some chunk of memory that they happened not
to be using for anything important...)

Anyway, probably better to hold off on applying this patch pending
further investigation. Patches 2/3 and 3/3 in this set should still
be fine to apply though.

-- PMM

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

[Cédric VINCENT]

Hi Peter & Martin,

On 2011-04-21 14:04:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> I've had a report from Martin Mohring that this patch breaks some
> programs which previously worked in linux-user mode (one wonders if
> they were overwriting some chunk of memory that they happened not
> to be using for anything important...)

Peter, actually your patch exposes two old bugs more explicitly (since
the error code is now correctly reported).  To complete your patchset,
just consider this contribution as "PATCH 4/3" ;)

Martin, I suppose you have encountered a problem with the memory
allocator used in Bash and Emacs.  I reduced the problem to the
test-case embedded in the commit message below.

Also, I wonder how this whole new implementation of the target brk
could help ScratchBox2? [1]

Regards,
Cédric

[1] http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg01817.html

PS: feel free to ask me for a separated patch if you don't [like to]
    use "git am --scissors"

8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----

There were two 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 following test
case:

    #include <unistd.h>       /* syscall(2),      */
    #include <sys/syscall.h>  /* SYS_brk,         */
    #include <stdio.h>        /* puts(3),         */
    #include <stdlib.h>       /* exit(3), EXIT_*, */

    int main()
    {
        int current_brk = 0;
        int new_brk;
        int failure = 0;

        void test(int increment) {
            static int test_number = 0;
            test_number++;

            new_brk = syscall(SYS_brk, current_brk + increment);
            if (new_brk == current_brk) {
                printf("test %d fails\n", test_number);
                failure++;
            }

            current_brk = new_brk;
        }

        /* Initialization.  */
        test(0);

        /* Does QEMU overlap host pages?  */
        test(HOST_PAGE_SIZE);
        test(HOST_PAGE_SIZE);

        /* Does QEMU allocate the same host page twice?  */
        test(-HOST_PAGE_SIZE);
        test(HOST_PAGE_SIZE);

        if (!failure) {
            printf("success\n");
            exit(EXIT_SUCCESS);
        }
        else {
            exit(EXIT_FAILURE);
        }
    }

Signed-off-by: Cédric VINCENT <cedric.vincent@st.com>
Reviewed-by: Christophe Guillon <christophe.guillon@st.com>
Cc: Riku Voipio <riku.voipio@iki.fi>
---
 linux-user/syscall.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b975730..be27f53 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -709,16 +709,17 @@ 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);
 }
 
 /* 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;
 
@@ -727,9 +728,8 @@ abi_long do_brk(abi_ulong new_brk)
     if (new_brk < target_original_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 the new brk is less than the highest page reserved to the
+     * target heap allocation, set it and we're done... */
     if (new_brk < brk_page) {
 	target_brk = new_brk;
     	return target_brk;
@@ -741,13 +741,14 @@ 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);
         return target_brk;
     } else if (mapped_addr != -1) {
         /* Mapped but at wrong address, meaning there wasn't actually
-- 
1.7.5.1

[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 <cedric.vincent@st.com>
Reviewed-by: Christophe Guillon <christophe.guillon@st.com>
Cc: Riku Voipio <riku.voipio@iki.fi>
---
 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

[Qemu-devel] [PATCH 2/3] arm-semi.c: Use correct check for failure of do_brk()

[Peter Maydell]

In the ARM semihosting implementation of SYS_HEAPINFO, use the correct
check for whether do_brk() has failed -- it does not return -1 but the
previous value of the break limit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 arm-semi.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arm-semi.c b/arm-semi.c
index e9e6f89..5a62d03 100644
--- a/arm-semi.c
+++ b/arm-semi.c
@@ -440,15 +440,16 @@ uint32_t do_arm_semihosting(CPUState *env)
             /* Some C libraries assume the heap immediately follows .bss, so
                allocate it using sbrk.  */
             if (!ts->heap_limit) {
-                long ret;
+                abi_ulong ret;
 
                 ts->heap_base = do_brk(0);
                 limit = ts->heap_base + ARM_ANGEL_HEAP_SIZE;
                 /* Try a big heap, and reduce the size if that fails.  */
                 for (;;) {
                     ret = do_brk(limit);
-                    if (ret != -1)
+                    if (ret >= limit) {
                         break;
+                    }
                     limit = (ts->heap_base >> 1) + (limit >> 1);
                 }
                 ts->heap_limit = limit;
-- 
1.7.1

[Qemu-devel] [PATCH 3/3] m68k-semi.c: Use correct check for failure of do_brk()

[Peter Maydell]

In the m68k semihosting implementation of HOSTED_INIT_SIM, use the correct
check for whether do_brk() has failed -- it does not return -1 but the
previous value of the break limit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 m68k-semi.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/m68k-semi.c b/m68k-semi.c
index 0371089..7fde10e 100644
--- a/m68k-semi.c
+++ b/m68k-semi.c
@@ -370,7 +370,7 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
         TaskState *ts = env->opaque;
         /* Allocate the heap using sbrk.  */
         if (!ts->heap_limit) {
-            long ret;
+            abi_ulong ret;
             uint32_t size;
             uint32_t base;
 
@@ -379,8 +379,9 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
             /* Try a big heap, and reduce the size if that fails.  */
             for (;;) {
                 ret = do_brk(base + size);
-                if (ret != -1)
+                if (ret >= (base + size)) {
                     break;
+                }
                 size >>= 1;
             }
             ts->heap_limit = base + size;
-- 
1.7.1