Message ID | 1321872000-14203-1-git-send-email-agraf@suse.de |
---|---|
State | New |
Headers | show |
On 21 November 2011 10:40, Alexander Graf <agraf@suse.de> wrote: > diff --git a/linux-user/strace.c b/linux-user/strace.c > index 90027a1..e30b005 100644 > --- a/linux-user/strace.c > +++ b/linux-user/strace.c > @@ -1515,14 +1515,19 @@ void > print_syscall_ret(int num, abi_long ret) > { > int i; > + char *errstr = NULL; > > for(i=0;i<nsyscalls;i++) > if( scnames[i].nr == num ) { > if( scnames[i].result != NULL ) { > scnames[i].result(&scnames[i],ret); > } else { > - if( ret < 0 ) { > - gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n", -ret, target_strerror(-ret)); > + if (ret < 0) { > + errstr = target_strerror(-ret); > + } > + if (errstr) { > + gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n", > + -ret, errstr); Should this really be printing -1 all the time when ret isn't -1 ? > } else { > gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret); > } print_syscall_ret_addr() also needs to handle NULL returns from target_strerror(). > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index f227097..c59d00e 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -731,6 +731,9 @@ static inline int is_error(abi_long ret) > > char *target_strerror(int err) > { > + if ((err >= ERRNO_TABLE_SIZE) && (err >= 0)) { > + return NULL; > + } > return strerror(target_to_host_errno(err)); shouldn't that be ((err >= ERRNO_TABLE_SIZE) || (err < 0)) ? -- PMM
On 21.11.2011, at 11:47, Peter Maydell wrote: > On 21 November 2011 10:40, Alexander Graf <agraf@suse.de> wrote: >> diff --git a/linux-user/strace.c b/linux-user/strace.c >> index 90027a1..e30b005 100644 >> --- a/linux-user/strace.c >> +++ b/linux-user/strace.c >> @@ -1515,14 +1515,19 @@ void >> print_syscall_ret(int num, abi_long ret) >> { >> int i; >> + char *errstr = NULL; >> >> for(i=0;i<nsyscalls;i++) >> if( scnames[i].nr == num ) { >> if( scnames[i].result != NULL ) { >> scnames[i].result(&scnames[i],ret); >> } else { >> - if( ret < 0 ) { >> - gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n", -ret, target_strerror(-ret)); >> + if (ret < 0) { >> + errstr = target_strerror(-ret); >> + } >> + if (errstr) { >> + gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n", >> + -ret, errstr); > > Should this really be printing -1 all the time when ret isn't -1 ? In linux-user, the syscall emulation functions return -errno which later gets translated to -1 with errno=-errno. That's why the code looks so weird. So yes, it's correct. > >> } else { >> gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret); >> } > > print_syscall_ret_addr() also needs to handle NULL > returns from target_strerror(). I don't think they can ever happen, since we're sure to use valid errnos there. But I can add a check. > >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index f227097..c59d00e 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -731,6 +731,9 @@ static inline int is_error(abi_long ret) >> >> char *target_strerror(int err) >> { >> + if ((err >= ERRNO_TABLE_SIZE) && (err >= 0)) { >> + return NULL; >> + } >> return strerror(target_to_host_errno(err)); > > shouldn't that be ((err >= ERRNO_TABLE_SIZE) || (err < 0)) ? Yes. Alex
On 21 November 2011 10:52, Alexander Graf <agraf@suse.de> wrote: > On 21.11.2011, at 11:47, Peter Maydell wrote: >> Should this really be printing -1 all the time when ret isn't -1 ? > > In linux-user, the syscall emulation functions return -errno which later > gets translated to -1 with errno=-errno. That's why the code looks so > weird. So yes, it's correct. Yes, but the syscall ABI (which is what we're implementing) returns -errno for errors; the errno variable is a purely userspace construct. I'd expected that the strace layer would be tracing exactly what the syscall ABI was. However it looks like standard userspace strace(1) also does this translation to -1 errno=thing, so it makes sense for us to do that too. -- PMM
diff --git a/linux-user/strace.c b/linux-user/strace.c index 90027a1..e30b005 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -1515,14 +1515,19 @@ void print_syscall_ret(int num, abi_long ret) { int i; + char *errstr = NULL; for(i=0;i<nsyscalls;i++) if( scnames[i].nr == num ) { if( scnames[i].result != NULL ) { scnames[i].result(&scnames[i],ret); } else { - if( ret < 0 ) { - gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n", -ret, target_strerror(-ret)); + if (ret < 0) { + errstr = target_strerror(-ret); + } + if (errstr) { + gemu_log(" = -1 errno=" TARGET_ABI_FMT_ld " (%s)\n", + -ret, errstr); } else { gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret); } diff --git a/linux-user/syscall.c b/linux-user/syscall.c index f227097..c59d00e 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -731,6 +731,9 @@ static inline int is_error(abi_long ret) char *target_strerror(int err) { + if ((err >= ERRNO_TABLE_SIZE) && (err >= 0)) { + return NULL; + } return strerror(target_to_host_errno(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 allowing failure to resolve invalid errnos into strings. Signed-off-by: Alexander Graf <agraf@suse.de> --- v1 -> v2: - propagate fault further down, so we display the negative value --- linux-user/strace.c | 9 +++++++-- linux-user/syscall.c | 3 +++ 2 files changed, 10 insertions(+), 2 deletions(-)