Zig: mremap 4096 causes segfault in 32-bit qemus

[marler8997]

Zig may or may not be the right place to put this bug, however, Zig is affected because it prevents us from calling mremap in our CI tests because we use qemu for non-native testing. As I'll explain, I'm fairly certain this is a bug in Qemu so we should file a bug with them for sure.

I added some code that called the mremap syscall, however, I was getting segfaults when zig was using qemu to run the code (only for 32-bit targets for some reason). I was able to narrow down the bug to 2, one to mmap, and one to mremap. And the sizes mattered as well, the first size had to be larger than 8192 and the second had to be 4096, if the second size was larger the segfault would not occur. I tested this with many configurations, I compiled the code in Zig, compiled in C with zig, and also reproduced the same issue by compling the C code with gcc. The only common factor I could find between all the cases is qemu. And note that it occurs whether I'm compiling and using qemu-i386 or qemu-arm, but not qemu-x86_64 (might be specific to 32-bit targets? or non-native targets?).

I've posted the C and Zig code to reproduce the issue below, save them to mremapbug.c and mremapbug.zig and see the following ways to reproduce:

Reproduce using gcc to compile C code

Note that if I compile with gcc, the segfault doesn't happen on qemu-x86_64, but does occur with qemu-i386. Make sure you have gcc-multilib installed to reproduce (nix would be nix-env -iA nixos.gcc_multi)

gcc -m32 mremapbug.c
# works
./a.out
# causes the segfault
qemu-i386 a.out

Reproduce using zig compiler to compile C code

zig build-exe -lc -target i386-linux-musl --c-source mremapbug.c 
# running outside qemu works fine:
./mremapbug
# running with qemu causes segfault
qemu-i386 mremapbug
# I used qemu gdb to ensure that the segfault occurs during the syscall to mremap

Reproduce using zig compiler to compile Zig

zig build-exe -target i386-linux-none mremapbug.zig
# works
./mremapbug
# causes segfault
qemu-i386 mremapbug

I also confirmed this segfault also occurs when compiling for arm and running with qemu-arm (though I didn't have an arm libc so I had to do it in zig).

zig build-exe -target arm-linux-none mremapbug.zig
# can't run ./mremapbug as I'm running on x86_64
# causes segfault
qemu-arm mremapbug

And keep in mind, if you change the second size to something else like 12288, no segfault. mremap might fail because it can't resize the buffer, but no segfault.

The C code to reproduce

#define _GNU_SOURCE
#include <stdlib.h>
#include <stdio.h>
#include <sys/mman.h>

int main(int argc, char *argv[])
{
  const size_t initial_size = 8192;

  printf("calling mmap, size=%llu\n", (unsigned long long)initial_size);
  void *mmap_ptr = mmap(NULL, initial_size,
                   PROT_READ | PROT_WRITE ,
                   MAP_PRIVATE | MAP_ANONYMOUS,
                   -1, 0);
  printf("mmap returned  : %p\n", mmap_ptr);
  if (mmap_ptr == MAP_FAILED) {
    perror("mmap");
    exit(1);
  }

  const size_t new_size = 4096;
  printf("calling mremap, size=%llu\n", (unsigned long long)new_size);
  void *remap_ptr = mremap(mmap_ptr, initial_size, new_size, 0);
  printf("mremap returned: %p\n", remap_ptr);
  if (remap_ptr != mmap_ptr) {
    perror("mreamap");
    exit(1);
  }
  printf("Success: pointers match\n");
}

The Zig code to reproduce

const std = @import("std");
const mem = std.mem;
const os = std.os;

pub fn main() !void {
    const initialSize = 8192;
    std.debug.warn("calling mmap, size={}\n", .{initialSize});
    const mmapResult = try os.mmap(
        null,
        initialSize,
        os.PROT_READ | os.PROT_WRITE,
        os.MAP_PRIVATE | os.MAP_ANONYMOUS,
        -1, 0);
    std.debug.warn("mmap returned  : {}\n", .{mmapResult.ptr});
    std.debug.assert(mmapResult.len == initialSize);
    const newSize = 4096;
    std.debug.warn("calling mremap, size={}\n", .{newSize});
    const remapResult = try mremap(mmapResult, newSize, 0);
    std.debug.warn("mremap returned: {}\n", .{remapResult});
    std.debug.assert(remapResult == mmapResult.ptr);
    std.debug.warn("Success: poitners match\n", .{});
}

// add functions to call mremap
pub fn sys_mremap(old_address: [*]align(mem.page_size) u8, old_size: usize, new_size: usize, flags: usize) usize {
    return os.linux.syscall4(.mremap, @ptrToInt(old_address), old_size, new_size, flags);
    // NOTE: using syscall5 doesn't seem to fix the issue
    //return os.linux.syscall5(.mremap, @ptrToInt(old_address), old_size, new_size, flags, 0);
}
fn mremap(buf: []align(mem.page_size) u8, newLen: usize, flags: usize) ![*]u8 {
    // only supported on linux as far as I know
    if (std.Target.current.os.tag != .linux) return error.OutOfMemory;
    const rc = sys_mremap(buf.ptr, buf.len, newLen, flags);
    switch (os.linux.getErrno(rc)) {
        0 => return @intToPtr([*]u8, rc),
        os.EAGAIN => return error.LockedMemoryLimitExceeded,
        os.EFAULT => return error.Fault,
        os.EINVAL => unreachable,
        os.ENOMEM => return error.OutOfMemory,
        else => |err| return os.unexpectedErrno(err),
    }
}

[marler8997]

I've reported the qemu bug here: https://bugs.launchpad.net/qemu/+bug/1876373

[LemonBoy]

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e378033797..caab62909e 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -708,7 +708,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
         if (prot == 0) {
             host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
             if (host_addr != MAP_FAILED && reserved_va && old_size > new_size) {
-                mmap_reserve(old_addr + old_size, new_size - old_size);
+                mmap_reserve(old_addr + old_size, old_size - new_size);
             }
         } else {
             errno = ENOMEM;

[marler8997]

@LemonBoy looking at the code, shouldn't it be something like this?

                if (new_size > old_size)
                    mmap_reserve(old_addr + old_size, new_size - old_size);
                else
                    mmap_shrink(old_addr + old_size, old_size - new_size); // does this exist?

Thanks for finding the line! This does look like an integer underflow when new_size < old_size

[LemonBoy]

The mapping is already shrunk, you call reserve to avoid returning part of the host VA back to the OS

[marler8997]

@LemonBoy I submitted a patch to the qemu-trivial mailing list (archive link here: https://lists.gnu.org/archive/html/qemu-trivial/2020-05/msg00000.html)

EDIT: this patch was wrong, submitted a new patch that matches @LemonBoy 's patch

I built and tested that it fixes the segfault issue. I just added an if (new_size > old_size) which was slightly different that your patch. Because new_size can also be greater than old_size I believe your fix would cause a segfault when new size was larger.

From the code, it looks like all mmap_reserve is doing is modifying the mmap flags on any new pages that were allocated during mremap. I believe this means that if we are shrinking the memory rather than expanding it, then there's no new pages to set flags for anyway so calling mmap_reserve isn't needed.

Let me know if I'm mistaken though.

I'll see if I can get my qemu patch in for zig.

[andrewrk]

I'd like to get this patch in our qemu binaries

I bet @mikdusan could help with that

[LemonBoy]

Your patch is wrong, you've added a check between old and new size that's opposite to the one in the if statement above. It works because the reserve isn't called anymore.

[marler8997]

@LemonBoy thanks again for the help.

Having another look, it looks like what's happening here is that this codepath that you've patched only occurs when the memory is being shrank (i.e. new_size < old_size). And since the memory has been unmapped, qemu is trying to "reserve" it so it doesn't got back to the OS. Now I think I understand what you were saying.

[marler8997]

Confirmed that this fixed my new code that calls mremap. Very nice:

Failing Case before qemu fix: https://dev.azure.com/ziglang/zig/_build/results?buildId=7045&view=logs&j=9512b82f-d185-50ea-ee23-d010bc14782f&t=ca25486c-bdcc-50c6-de82-a9ccd6b64318
Passing case after fix: https://dev.azure.com/ziglang/zig/_build/results?buildId=7085&view=logs&j=9512b82f-d185-50ea-ee23-d010bc14782f&t=ca25486c-bdcc-50c6-de82-a9ccd6b64318

[marler8997]

I have no idea how @LemonBoy came up with the fix so fast...WTF?

[FireFox317]

@LemonBoy is a beast🔥