[v2] linux-user: Fix brk() to release pages

Commit Message

Helge Deller Dec. 23, 2022, 9:03 p.m. UTC

The current brk() implementation does not de-allocate pages if a lower
address is given compared to earlier brk() calls.
But according to the manpage, brk() shall deallocate memory in this case
and currently it breaks a real-world application, specifically building
the debian gcl package in qemu-user.

Fix this issue by reworking the qemu brk() implementation.

Tested with the C-code testcase included in qemu commit 4d1de87c750, and
by building debian package of gcl in a hppa-linux guest on a x86-64
host.

Signed-off-by: Helge Deller <[e-mail]>

---
v2:
- Fixed return value of brk(). The v1 version wrongly page-aligned the
  provided address, while userspace expects unmodified the address
  returned unmodified.

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 4fee882cd7..2fcb6dba06 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -838,49 +838,59 @@  static inline int host_to_target_sock_type(int host_type)
 }

 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);
+    target_brk = new_brk;
+    brk_page = HOST_PAGE_ALIGN(new_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_long do_brk(abi_ulong brk_val)
 {
     abi_long mapped_addr;
     abi_ulong new_alloc_size;
+    abi_ulong new_brk, new_host_brk_page;

     /* brk pointers are always untagged */

+    /* return old brk value on zero brk_val */
+    if (!brk_val || brk_val == target_brk) {
+        return target_brk;
+    }
+
+    new_brk = TARGET_PAGE_ALIGN(brk_val);
+    new_host_brk_page = HOST_PAGE_ALIGN(brk_val);
+
     DEBUGF_BRK("do_brk(" TARGET_ABI_FMT_lx ") -> ", new_brk);

-    if (!new_brk) {
+    /* brk_val and old target_brk might be on the same page */
+    if (new_brk == TARGET_PAGE_ALIGN(target_brk)) {
+        if (brk_val > target_brk) {
+            /* empty remaining bytes in (possibly larger) host page */
+            memset(g2h_untagged(target_brk), 0, new_host_brk_page - target_brk);
+        }
         DEBUGF_BRK(TARGET_ABI_FMT_lx " (!new_brk)\n", target_brk);
-        return target_brk;
-    }
-    if (new_brk < target_original_brk) {
-        DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk < target_original_brk)\n",
-                   target_brk);
-        return target_brk;
+        target_brk = brk_val;
+        return brk_val;
     }

-    /* 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_untagged(target_brk), 0, new_brk - target_brk);
-        }
-	target_brk = new_brk;
+    /* Release heap if necesary */
+    if (new_brk < target_brk) {
+        /* empty remaining bytes in (possibly larger) host page */
+        memset(g2h_untagged(brk_val), 0, new_host_brk_page - brk_val);
+
+        /* free unused host pages and set new brk_page */
+        target_munmap(new_host_brk_page, brk_page - new_host_brk_page);
+        brk_page = new_host_brk_page;
+
+	target_brk = brk_val;
         DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk <= brk_page)\n", target_brk);
-	return target_brk;
+	return brk_val;
     }

     /* We need to allocate more memory after the brk... Note that
@@ -889,10 +899,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);
-    mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
+    new_alloc_size = new_host_brk_page - brk_page;
+    if (new_alloc_size) {
+        mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
                                         PROT_READ|PROT_WRITE,
                                         MAP_ANON|MAP_PRIVATE, 0, 0));
+    } else {
+        mapped_addr = brk_page;
+    }

     if (mapped_addr == brk_page) {
         /* Heap contents are initialized to zero, as for anonymous
@@ -905,10 +919,10 @@  abi_long do_brk(abi_ulong new_brk)
         memset(g2h_untagged(target_brk), 0, brk_page - target_brk);

         target_brk = new_brk;
-        brk_page = HOST_PAGE_ALIGN(target_brk);
+        brk_page = new_host_brk_page;
         DEBUGF_BRK(TARGET_ABI_FMT_lx " (mapped_addr == brk_page)\n",
             target_brk);
-        return target_brk;
+        return brk_val;
     } else if (mapped_addr != -1) {
         /* Mapped but at wrong address, meaning there wasn't actually
          * enough space for this brk.