QEMU

arm-semi mishandling SYS_HEAPINFO

Bug #656285 reported by Stephen Clarke
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
QEMU
Fix Released
Undecided
Unassigned

Bug Description

I am running qemu-arm on a 32-bit fedora-7 i386 machine:

$ /home/bri0633/users/clarkes/qemu/build/arm-linux-user/qemu-arm --version
qemu-arm version 0.12.3, Copyright (c) 2003-2008 Fabrice Bellard

When I try to run an arm semi-hosted executable, I sometimes get unexpected segv and sometimes not, depending on the executable. The symptom is:

$ /home/bri0633/users/clarkes/qemu/build/arm-linux-user/qemu-arm -cpu cortex-a9 -- a.out
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault

It appear to be because of the handling of the SYS_HEAPINFO syscall in arm-semi.c. There it tries to allocate 128M for the heap by calling do_brk() which calls target_mmap(). This is the DEBUG_MMAP diagnostic:

mmap: start=0x00009000 len=0x08001000 prot=rw- flags=MAP_FIXED MAP_ANON MAP_PRIVATE fd=0 offset=00000000

but this mmap is failing because there are shared libraries (and the gate page) mapped there:

$ ldd /home/bri0633/users/clarkes/qemu/build/arm-linux-user/qemu-arm
        linux-gate.so.1 => (0x00880000)
        librt.so.1 => /lib/librt.so.1 (0x03409000)
        libpthread.so.0 => /lib/libpthread.so.0 (0x00d7d000)
        libm.so.6 => /lib/libm.so.6 (0x00d4b000)
        libc.so.6 => /lib/libc.so.6 (0x00bf5000)
        /lib/ld-linux.so.2 (0x00bd6000)

However, it seems that the code in arm-semi.c does not interpret the result of do_brk() correctly, and thinks that the mapping succeeded.
The following patch appears to fix the problem:

$ diff -u arm-semi.c.orig arm-semi.c
--- arm-semi.c.orig 2010-09-21 13:19:15.000000000 +0100
+++ arm-semi.c 2010-10-07 13:23:13.000000000 +0100
@@ -475,7 +475,7 @@
                 /* 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);
                 }

Do you think this is a genuine bug?
Steve.

Revision history for this message
Brian Harring (ferringb) wrote :

Look through linux-user/syscall.c; looks like the flaw is more in do_brk itself. Invocations of do_brk *appear* to all assume that it's basically brk like in it's behaviour- -1 on failure, else a non-negative value of what the size now is. So... your patch is breaking away from proper behaviour, and won't handle when the pre-existing brk is greater than the requested (purely due to binding it to limit).

Looking at that code, it looks like ret should be tracked on success, since that is the actual size (in the case of larger than requested). Further, looks like the issue is probably in do_brk itself; when it fails, exempting the alpha case, it returns the unmodified target_brk... which likely isn't going to by the code flow.

Revision history for this message
Peter Maydell (pmaydell) wrote : Re: [Qemu-devel] [Bug 656285] Re: arm-semi mishandling SYS_HEAPINFO

On 27 November 2010 19:29, Brian Harring <email address hidden> wrote:
> Look through linux-user/syscall.c; looks like the flaw is more in do_brk
> itself.  Invocations of do_brk *appear* to all assume that it's
> basically brk like in it's behaviour- -1 on failure, else a non-negative
> value of what the size now is.

I think this is wrong. The primary user of do_brk() is the Linux
user system call emulation, so do_brk()'s semantics follow
those of the kernel implemented syscall. (This is different from
those of the brk() library routine, hence the confusion.) That means
it returns the old brk value on failure, not -1. [This is documented
in the NOTES section of the Linux brk(2) manpage, or you can
look at the kernel sources.]

You could argue that this is a bit unhelpful for platform-independent
code like the ARM semihosting routines which end up coding to
an API which might be platform-dependent between linux-user
and some-other-user modes...

-- PMM

Revision history for this message
Brian Harring (ferringb) wrote :

Pardon, badly phrased on my part- basically we can get either -1 or old. If we changed that conditional w/in arm-semi.c to:

if (-1 != limit && limit != ts->heap_base) {

that should do the trick in a cross platform way, although we'll have to tweak the for loop to handle when limit degrades to 0. Note that this isn't the only spot that has the -1 assumption- m68-semi.c is the same. Also, linux-user/m68k-sim.c looks a bit odd in light of this discussion (that one I don't quite get- it checks for -ENOMEM which is supposed to be an alpha only return).

Revision history for this message
Peter Maydell (pmaydell) wrote :

I've just submitted a patchset which I think fixes this bug (among others):

http://patchwork.ozlabs.org/patch/91789/ [1/3] linux-user: Don't use MAP_FIXED in do_brk()
http://patchwork.ozlabs.org/patch/91790/ [2/3] arm-semi.c: Use correct check for failure of do_brk()
http://patchwork.ozlabs.org/patch/91793/ [3/3] m68k-semi.c: Use correct check for failure of do_brk()

Revision history for this message
Peter Maydell (pmaydell) wrote :

The patches I mention in commit #4 (and also a fix by Cedric Vincent for some other brk related bugs) have now been committed to qemu master.

Changed in qemu:
status: New → Fix Committed
Revision history for this message
Peter Maydell (pmaydell) wrote :

QEMU 0.15.0 has been released with a fix for this bug.

Changed in qemu:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.