diff mbox

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

Message ID 1303140866-29348-2-git-send-email-peter.maydell@linaro.org
State Accepted
Commit 00faf08c951fcc351467faac5697307a86edb077
Headers show

Commit Message

Peter Maydell April 18, 2011, 3:34 p.m. UTC 
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

Comments

Peter Maydell April 21, 2011, 2:04 p.m. UTC | #1 
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
diff mbox

Patch

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;
 }