Message ID | 1321790786-28518-1-git-send-email-agraf@suse.de |
---|---|
State | New |
Headers | show |
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
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
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;
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(-)