Re: [PATCH v2 2/2] linux-user: fix print_syscall_err() when syscall returned value is negative

[Laurent Vivier]

Le 08/07/2020 à 17:24, Laurent Vivier a écrit :
> print_syscall_err() relies on the sign of the returned value to know
> if it is an errno value or not.
> 
> But in some cases the returned value can have the most signicant bit
> set without being an errno.
> 
> This patch restores previous behaviour that was also checking if
> we can decode the errno to validate it.
> 
> This patch fixes this kind of problem (qemu-m68k):
> 
>   root@sid:/# QEMU_STRACE= ls
>   3 brk(NULL) = -1 errno=21473607683 uname(0x407fff8a) = 0
> 
> to become:
> 
>   root@sid:/# QEMU_STRACE= ls
>   3 brk(NULL) = 0x8001e000
>   3 uname(0xffffdf8a) = 0
> 
> Fixes: c84be71f6854 ("linux-user: Extend strace support to enable argument 
> printing after syscall execution")
> Cc: Filip.Bozuta@syrmia.com
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/strace.c | 36 +++++++++++++-----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index b42664bbd180..17f2554643f0 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -724,19 +724,20 @@ print_ipc(const struct syscallname *name,
>   * Variants for the return value output function
>   */
>  
> -static void
> +static bool
>  print_syscall_err(abi_long ret)
>  {
> -    const char *errstr = NULL;
> +    const char *errstr;
>  
>      qemu_log(" = ");
>      if (ret < 0) {
> -        qemu_log("-1 errno=%d", (int)-ret);
>          errstr = target_strerror(-ret);
>          if (errstr) {
> -            qemu_log(" (%s)", errstr);
> +            qemu_log("-1 errno=%d (%s)", (int)-ret, errstr);
> +            return true;
>          }
>      }
> +    return false;
>  }
>  
>  static void
> @@ -744,11 +745,10 @@ print_syscall_ret_addr(const struct syscallname *name, 
> abi_long ret,
>                         abi_long arg0, abi_long arg1, abi_long arg2,
>                         abi_long arg3, abi_long arg4, abi_long arg5)
>  {
> -    print_syscall_err(ret);
> -
> -    if (ret >= 0) {
> -        qemu_log("0x" TARGET_ABI_FMT_lx "\n", ret);
> +    if (!print_syscall_err(ret)) {
> +        qemu_log("0x" TARGET_ABI_FMT_lx, ret);
>      }
> +    qemu_log("\n");
>  }
>  
>  #if 0 /* currently unused */
> @@ -765,9 +765,7 @@ print_syscall_ret_newselect(const struct syscallname 
> *name, abi_long ret,
>                              abi_long arg0, abi_long arg1, abi_long arg2,
>                              abi_long arg3, abi_long arg4, abi_long arg5)
>  {
> -    print_syscall_err(ret);
> -
> -    if (ret >= 0) {
> +    if (!print_syscall_err(ret)) {
>          qemu_log(" = 0x" TARGET_ABI_FMT_lx " (", ret);
>          print_fdset(arg0, arg1);
>          qemu_log(",");
> @@ -796,9 +794,7 @@ print_syscall_ret_adjtimex(const struct syscallname 
> *name, abi_long ret,
>                             abi_long arg0, abi_long arg1, abi_long arg2,
>                             abi_long arg3, abi_long arg4, abi_long arg5)
>  {
> -    print_syscall_err(ret);
> -
> -    if (ret >= 0) {
> +    if (!print_syscall_err(ret)) {
>          qemu_log(TARGET_ABI_FMT_ld, ret);
>          switch (ret) {
>          case TARGET_TIME_OK:
> @@ -833,9 +829,7 @@ print_syscall_ret_listxattr(const struct syscallname 
> *name, abi_long ret,
>                              abi_long arg0, abi_long arg1, abi_long arg2,
>                              abi_long arg3, abi_long arg4, abi_long arg5)
>  {
> -    print_syscall_err(ret);
> -
> -    if (ret >= 0) {
> +    if (!print_syscall_err(ret)) {
>          qemu_log(TARGET_ABI_FMT_ld, ret);
>          qemu_log(" (list = ");
>          if (arg1 != 0) {
> @@ -866,9 +860,7 @@ print_syscall_ret_ioctl(const struct syscallname *name, 
> abi_long ret,
>                          abi_long arg0, abi_long arg1, abi_long arg2,
>                          abi_long arg3, abi_long arg4, abi_long arg5)
>  {
> -    print_syscall_err(ret);
> -
> -    if (ret >= 0) {
> +    if (!print_syscall_err(ret)) {
>          qemu_log(TARGET_ABI_FMT_ld, ret);
>  
>          const IOCTLEntry *ie;
> @@ -3189,9 +3181,7 @@ print_syscall_ret(int num, abi_long ret,
>                                    arg1, arg2, arg3,
>                                    arg4, arg5, arg6);
>              } else {
> -                print_syscall_err(ret);
> -
> -                if (ret >= 0) {
> +                if (!print_syscall_err(ret)) {
>                      qemu_log(TARGET_ABI_FMT_ld, ret);
>                  }
>                  qemu_log("\n");
> 

Applied to my linux-user-for-5.1 branch.

Richard: I agree with your comment but I'd like to fix the regression
before the hard freeze. Filip is currently working on some improvements
in strace functions that will allow to have access to env variable. So
we will be able to have a target-specific test as you suggested.

Thanks,
Laurent