Patchwork [Bug,656285] Re: arm-semi mishandling SYS_HEAPINFO

login
register
mail settings
Submitter Brian Harring
Date Nov. 27, 2010, 7:29 p.m.
Message ID <20101127192930.5365.13234.malone@potassium.ubuntu.com>
Download mbox | patch
Permalink /patch/73295/
State New
Headers show

Comments

Brian Harring - Nov. 27, 2010, 7:29 p.m.
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.
Peter Maydell - Nov. 27, 2010, 8:06 p.m.
On 27 November 2010 19:29, Brian Harring <ferringb@gmail.com> 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

Patch

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