[PATCH v2 0/2] linux-user: fix print_syscall_err()

[PATCH v2 0/2] linux-user: fix print_syscall_err()

[Laurent Vivier]

This function has been introduced to manage in a generic way the error
code of the syscall in the strace output.

But it has introduced a regression regarding two previous commits:

2a7e12455c1d ("linux-user/strace.c: Correct errno printing for mmap etc")
   that intoduced the use of "-ret" rather than of "errno"
962b289ef350 ("linux-user: fix QEMU_STRACE=1 segfault")
   that checks "-ret" is a valid error number

That series fixes that.

v2: add the patch to check "-ret" is valid

Laurent Vivier (2):
  linux-user: fix the errno value in print_syscall_err()
  linux-user: fix print_syscall_err() when syscall returned value is
    negative

 linux-user/strace.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] linux-user: fix the errno value in print_syscall_err()

[Laurent Vivier]

errno of the target is returned as a negative value by the syscall,
not in the host errno variable.

The emulation of the target syscall can return an error while the
host doesn't set an errno value. Target errnos and host errnos can
also differ in some cases.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 5235b2260cdd..b42664bbd180 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -731,7 +731,7 @@ print_syscall_err(abi_long ret)
 
     qemu_log(" = ");
     if (ret < 0) {
-        qemu_log("-1 errno=%d", errno);
+        qemu_log("-1 errno=%d", (int)-ret);
         errstr = target_strerror(-ret);
         if (errstr) {
             qemu_log(" (%s)", errstr);
-- 
2.26.2

Re: [PATCH v2 1/2] linux-user: fix the errno value in print_syscall_err()

[Richard Henderson]

On 7/8/20 8:24 AM, Laurent Vivier wrote:
> errno of the target is returned as a negative value by the syscall,
> not in the host errno variable.
> 
> The emulation of the target syscall can return an error while the
> host doesn't set an errno value. Target errnos and host errnos can
> also differ in some cases.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 1/2] linux-user: fix the errno value in print_syscall_err()

[Philippe Mathieu-Daudé]

On 7/8/20 5:24 PM, Laurent Vivier wrote:
> errno of the target is returned as a negative value by the syscall,
> not in the host errno variable.
> 
> The emulation of the target syscall can return an error while the
> host doesn't set an errno value. Target errnos and host errnos can
> also differ in some cases.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 5235b2260cdd..b42664bbd180 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -731,7 +731,7 @@ print_syscall_err(abi_long ret)
>  
>      qemu_log(" = ");
>      if (ret < 0) {
> -        qemu_log("-1 errno=%d", errno);
> +        qemu_log("-1 errno=%d", (int)-ret);
>          errstr = target_strerror(-ret);
>          if (errstr) {
>              qemu_log(" (%s)", errstr);
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 1/2] linux-user: fix the errno value in print_syscall_err()

[Filip Bozuta]

On 8.7.20. 17:24, Laurent Vivier wrote:
> errno of the target is returned as a negative value by the syscall,
> not in the host errno variable.
>
> The emulation of the target syscall can return an error while the
> host doesn't set an errno value. Target errnos and host errnos can
> also differ in some cases.
>
> 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 5235b2260cdd..b42664bbd180 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -731,7 +731,7 @@ print_syscall_err(abi_long ret)
>   
>       qemu_log(" = ");
>       if (ret < 0) {
> -        qemu_log("-1 errno=%d", errno);
> +        qemu_log("-1 errno=%d", (int)-ret);
>           errstr = target_strerror(-ret);
>           if (errstr) {
>               qemu_log(" (%s)", errstr);
Reviewed-by: Filip Bozuta <Filip.Bozuta@syrmia.com>

Re: [PATCH v2 1/2] linux-user: fix the errno value in print_syscall_err()

[Laurent Vivier]

Le 08/07/2020 à 17:24, Laurent Vivier a écrit :
> errno of the target is returned as a negative value by the syscall,
> not in the host errno variable.
> 
> The emulation of the target syscall can return an error while the
> host doesn't set an errno value. Target errnos and host errnos can
> also differ in some cases.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 5235b2260cdd..b42664bbd180 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -731,7 +731,7 @@ print_syscall_err(abi_long ret)
>  
>      qemu_log(" = ");
>      if (ret < 0) {
> -        qemu_log("-1 errno=%d", errno);
> +        qemu_log("-1 errno=%d", (int)-ret);
>          errstr = target_strerror(-ret);
>          if (errstr) {
>              qemu_log(" (%s)", errstr);
> 

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

Thanks,
Laurent

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

[Laurent Vivier]

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");
-- 
2.26.2

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

[Richard Henderson]

On 7/8/20 8:24 AM, Laurent Vivier wrote:
> -static void
> +static bool
>  print_syscall_err(abi_long ret)
>  {
> -    const char *errstr = NULL;
> +    const char *errstr;
>  
>      qemu_log(" = ");
>      if (ret < 0) {

This should be a target-specific test.

E.g. on most asm-generic I'm pretty sure this should be

    if ((abi_ulong)ret > -(abi_ulong)512)

whereas for Alpha it should be

    /*
     * Syscall writes 0 to V0 to bypass error check, similar
     * to how this is handled internal to Linux kernel.
     */
    if (ret < 0 && env->ir[IR_V0] != 0)


r~

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

[Laurent Vivier]

Le 08/07/2020 à 17:52, Richard Henderson a écrit :
> On 7/8/20 8:24 AM, Laurent Vivier wrote:
>> -static void
>> +static bool
>>  print_syscall_err(abi_long ret)
>>  {
>> -    const char *errstr = NULL;
>> +    const char *errstr;
>>  
>>      qemu_log(" = ");
>>      if (ret < 0) {
> 
> This should be a target-specific test.
> 
> E.g. on most asm-generic I'm pretty sure this should be
> 
>     if ((abi_ulong)ret > -(abi_ulong)512)

I think the test in target_strerror() gives the same result:

    if ((err >= ERRNO_TABLE_SIZE) || (err < 0)) {
        return NULL;
    }

and it also ensures we don't overflow when we will access
target_to_host_errno_table[].

It's why we rely on errstr to know if the errno is valid or not
(we might also remove the "if (ret < 0)" in print_syscall_err).

> whereas for Alpha it should be
> 
>     /*
>      * Syscall writes 0 to V0 to bypass error check, similar
>      * to how this is handled internal to Linux kernel.
>      */
>     if (ret < 0 && env->ir[IR_V0] != 0)

We don't have access to "env" in strace.c.

it's an improvement regarding the code that has been modified.
If we want it I think it should be added in a separate patch.

Thanks,
Laurent

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