Patchwork linux-user: fix QEMU_STRACE=1 segfault

login
register
mail settings
Submitter Alexander Graf
Date Nov. 20, 2011, 12:06 p.m.
Message ID <1321790786-28518-1-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/126652/
State New
Headers show

Comments

Alexander Graf - Nov. 20, 2011, 12:06 p.m.
While debugging some issues with QEMU_STRACE I stumbled over segmentation
faults that were pretty reproducible. Turns out we tried to treat a
normal return value as errno, resulting in an access over array boundaries
for the resolution.

Fix this by hard-mapping values above valid errnos to the original value.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/syscall.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Peter Maydell - Nov. 20, 2011, 3:31 p.m.
On 20 November 2011 12:06, Alexander Graf <agraf@suse.de> wrote:
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -711,6 +711,9 @@ static inline int host_to_target_errno(int err)
>
>  static inline int target_to_host_errno(int err)
>  {
> +    if (err >= ERRNO_TABLE_SIZE) {
> +        return err;
> +    }
>     if (target_to_host_errno_table[err])
>         return target_to_host_errno_table[err];
>     return err;

Really strace shouldn't be assuming all negative values
are errnos: the code has a "print in one format for errnos,
print in another if we're assuming it's an address", so we
should have a way for the stracing code to be making the
right "errno or not?" decision, so we can print these normal
return values properly.

Since target_to_host_errno() is only used by target_strerror()
and target_strerror() is only used by the strace code we should
just change its API to something easier for the strace code to
use. How about having target_strerror() return NULL for "this
doesn't look like an errno", and then the strace layer prints
the plain address or "address (errno string)" accordingly?

OTOH host_to_target_errno() could probably use a bounds check.

-- PMM
Alexander Graf - Nov. 20, 2011, 4:18 p.m.
On 20.11.2011, at 16:31, Peter Maydell wrote:

> On 20 November 2011 12:06, Alexander Graf <agraf@suse.de> wrote:
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -711,6 +711,9 @@ static inline int host_to_target_errno(int err)
>> 
>>  static inline int target_to_host_errno(int err)
>>  {
>> +    if (err >= ERRNO_TABLE_SIZE) {
>> +        return err;
>> +    }
>>     if (target_to_host_errno_table[err])
>>         return target_to_host_errno_table[err];
>>     return err;
> 
> Really strace shouldn't be assuming all negative values
> are errnos: the code has a "print in one format for errnos,
> print in another if we're assuming it's an address", so we
> should have a way for the stracing code to be making the
> right "errno or not?" decision, so we can print these normal
> return values properly.

Yep. I don't want to have to trace segfaults when it goes wrong though :).

> Since target_to_host_errno() is only used by target_strerror()
> and target_strerror() is only used by the strace code we should
> just change its API to something easier for the strace code to
> use. How about having target_strerror() return NULL for "this
> doesn't look like an errno", and then the strace layer prints
> the plain address or "address (errno string)" accordingly?

I like that idea, yup.

> OTOH host_to_target_errno() could probably use a bounds check.

Maybe I'm too spoiled by JITed programming languages, but I really like it when array accessor functions check for sanity. I don't mind if we assert() it though, so the bug is easier to spot next time around.


Alex

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f227097..312aec5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -711,6 +711,9 @@  static inline int host_to_target_errno(int err)
 
 static inline int target_to_host_errno(int err)
 {
+    if (err >= ERRNO_TABLE_SIZE) {
+        return err;
+    }
     if (target_to_host_errno_table[err])
         return target_to_host_errno_table[err];
     return err;