[Qemu-devel] [RFC PATCH] semihosting: split console_out intro string and char versions

[Qemu-devel] [RFC PATCH] semihosting: split console_out intro string and char versions

[Alex Bennée]

This is ostensibly to avoid the weirdness of len looking like it might
come from a guest and sometimes being used. While we are at it fix up
the error checking for the arm-linux-user implementation of the API
which got flagged up by Coverity (CID 1401700).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/semihosting/console.c         | 34 +++++++++++++++++++++++---------
 include/hw/semihosting/console.h | 25 +++++++++++++++++------
 linux-user/arm/semihost.c        | 28 ++++++++++++++++++++++++--
 target/arm/arm-semi.c            |  4 ++--
 4 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index 466ea6dade7..4a5758972db 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -36,26 +36,24 @@ int qemu_semihosting_log_out(const char *s, int len)
 /*
  * A re-implementation of lock_user_string that we can use locally
  * instead of relying on softmmu-semi. Hopefully we can deprecate that
- * in time. We either copy len bytes if specified or until we find a NULL.
+ * in time. Copy string until we find a 0 or address error.
  */
-static GString *copy_user_string(CPUArchState *env, target_ulong addr, int len)
+static GString *copy_user_string(CPUArchState *env, target_ulong addr)
 {
     CPUState *cpu = ENV_GET_CPU(env);
-    GString *s = g_string_sized_new(len ? len : 128);
+    GString *s = g_string_sized_new(128);
     uint8_t c;
-    bool done;
 
     do {
         if (cpu_memory_rw_debug(cpu, addr++, &c, 1, 0) == 0) {
             s = g_string_append_c(s, c);
-            done = len ? s->len == len : c == 0;
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "%s: passed inaccessible address " TARGET_FMT_lx,
                           __func__, addr);
-            done = true;
+            break;
         }
-    } while (!done);
+    } while (c!=0);
 
     return s;
 }
@@ -68,9 +66,9 @@ static void semihosting_cb(CPUState *cs, target_ulong ret, target_ulong err)
     }
 }
 
-int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len)
+int qemu_semihosting_console_outs(CPUArchState *env, target_ulong addr)
 {
-    GString *s = copy_user_string(env, addr, len);
+    GString *s = copy_user_string(env, addr);
     int out = s->len;
 
     if (use_gdb_syscalls()) {
@@ -82,3 +80,21 @@ int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len)
     g_string_free(s, true);
     return out;
 }
+
+void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
+{
+    CPUState *cpu = ENV_GET_CPU(env);
+    uint8_t c;
+
+    if (cpu_memory_rw_debug(cpu, addr, &c, 1, 0) == 0) {
+        if (use_gdb_syscalls()) {
+            gdb_do_syscall(semihosting_cb, "write,2,%x,%x", addr, 1);
+        } else {
+            qemu_semihosting_log_out((const char *) &c, 1);
+        }
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: passed inaccessible address " TARGET_FMT_lx,
+                      __func__, addr);
+    }
+}
diff --git a/include/hw/semihosting/console.h b/include/hw/semihosting/console.h
index 30e66ae20aa..3a4fba75905 100644
--- a/include/hw/semihosting/console.h
+++ b/include/hw/semihosting/console.h
@@ -10,17 +10,30 @@
 #define _SEMIHOST_CONSOLE_H_
 
 /**
- * qemu_semihosting_console_out:
+ * qemu_semihosting_console_outs:
  * @env: CPUArchState
- * @s: host address of guest string
- * @len: length of string or 0 (string is null terminated)
+ * @s: host address of null terminated guest string
  *
- * Send a guest string to the debug console. This may be the remote
- * gdb session if a softmmu guest is currently being debugged.
+ * Send a null terminated guest string to the debug console. This may
+ * be the remote gdb session if a softmmu guest is currently being
+ * debugged.
  *
  * Returns: number of bytes written.
  */
-int qemu_semihosting_console_out(CPUArchState *env, target_ulong s, int len);
+int qemu_semihosting_console_outs(CPUArchState *env, target_ulong s);
+
+/**
+ * qemu_semihosting_console_outc:
+ * @env: CPUArchState
+ * @s: host address of null terminated guest string
+ *
+ * Send single character from guest memory to the debug console. This
+ * may be the remote gdb session if a softmmu guest is currently being
+ * debugged.
+ *
+ * Returns: nothing
+ */
+void qemu_semihosting_console_outc(CPUArchState *env, target_ulong c);
 
 /**
  * qemu_semihosting_log_out:
diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
index 9554102a855..b7cdc21f832 100644
--- a/linux-user/arm/semihost.c
+++ b/linux-user/arm/semihost.c
@@ -15,10 +15,34 @@
 #include "hw/semihosting/console.h"
 #include "qemu.h"
 
-int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len)
+int qemu_semihosting_console_outs(CPUArchState *env, target_ulong addr)
 {
+    int len;
     void *s = lock_user_string(addr);
-    len = write(STDERR_FILENO, s, len ? len : strlen(s));
+    if (!s) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: passed inaccessible address " TARGET_FMT_lx,
+                      __func__, addr);
+        return 0;
+    }
+
+    len = write(STDERR_FILENO, s, strlen(s));
     unlock_user(s, addr, 0);
     return len;
 }
+
+void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
+{
+    char c;
+
+    if (get_user_u8(c, addr)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: passed inaccessible address " TARGET_FMT_lx,
+                      __func__, addr);
+    } else {
+        if (write(STDERR_FILENO, &c, 1) != 1) {
+            qemu_log_mask(LOG_UNIMP, "%s: unexpected write to stdout failure",
+                          __func__);
+        }
+    }
+}
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 53e807ab721..8844da8da38 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -315,10 +315,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return set_swi_errno(ts, close(arg0));
         }
     case TARGET_SYS_WRITEC:
-        qemu_semihosting_console_out(env, args, 1);
+        qemu_semihosting_console_outc(env, args);
         return 0xdeadbeef;
     case TARGET_SYS_WRITE0:
-        return qemu_semihosting_console_out(env, args, 0);
+        return qemu_semihosting_console_outs(env, args);
     case TARGET_SYS_WRITE:
         GET_ARG(0);
         GET_ARG(1);
-- 
2.20.1

Re: [Qemu-devel] [RFC PATCH] semihosting: split console_out intro string and char versions

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190530143916.20255-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [RFC PATCH] semihosting: split console_out intro string and char versions
Type: series
Message-id: 20190530143916.20255-1-alex.bennee@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190530143916.20255-1-alex.bennee@linaro.org -> patchew/20190530143916.20255-1-alex.bennee@linaro.org
Switched to a new branch 'test'
98c557b357 semihosting: split console_out intro string and char versions

=== OUTPUT BEGIN ===
ERROR: spaces required around that '!=' (ctx:VxV)
#47: FILE: hw/semihosting/console.c:56:
+    } while (c!=0);
               ^

total: 1 errors, 0 warnings, 147 lines checked

Commit 98c557b35708 (semihosting: split console_out intro string and char versions) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190530143916.20255-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [RFC PATCH] semihosting: split console_out intro string and char versions

[Laurent Vivier]

Le 30/05/2019 à 16:39, Alex Bennée a écrit :
> This is ostensibly to avoid the weirdness of len looking like it might
> come from a guest and sometimes being used. While we are at it fix up
> the error checking for the arm-linux-user implementation of the API
> which got flagged up by Coverity (CID 1401700).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/semihosting/console.c         | 34 +++++++++++++++++++++++---------
>  include/hw/semihosting/console.h | 25 +++++++++++++++++------
>  linux-user/arm/semihost.c        | 28 ++++++++++++++++++++++++--
>  target/arm/arm-semi.c            |  4 ++--
>  4 files changed, 72 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
> index 466ea6dade7..4a5758972db 100644
> --- a/hw/semihosting/console.c
> +++ b/hw/semihosting/console.c
> @@ -36,26 +36,24 @@ int qemu_semihosting_log_out(const char *s, int len)
>  /*
>   * A re-implementation of lock_user_string that we can use locally
>   * instead of relying on softmmu-semi. Hopefully we can deprecate that
> - * in time. We either copy len bytes if specified or until we find a NULL.
> + * in time. Copy string until we find a 0 or address error.
>   */
> -static GString *copy_user_string(CPUArchState *env, target_ulong addr, int len)
> +static GString *copy_user_string(CPUArchState *env, target_ulong addr)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> -    GString *s = g_string_sized_new(len ? len : 128);
> +    GString *s = g_string_sized_new(128);
>      uint8_t c;
> -    bool done;
>  
>      do {
>          if (cpu_memory_rw_debug(cpu, addr++, &c, 1, 0) == 0) {
>              s = g_string_append_c(s, c);
> -            done = len ? s->len == len : c == 0;
>          } else {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "%s: passed inaccessible address " TARGET_FMT_lx,
>                            __func__, addr);
> -            done = true;
> +            break;
>          }
> -    } while (!done);
> +    } while (c!=0);
>  
>      return s;
>  }
> @@ -68,9 +66,9 @@ static void semihosting_cb(CPUState *cs, target_ulong ret, target_ulong err)
>      }
>  }
>  
> -int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len)
> +int qemu_semihosting_console_outs(CPUArchState *env, target_ulong addr)
>  {
> -    GString *s = copy_user_string(env, addr, len);
> +    GString *s = copy_user_string(env, addr);
>      int out = s->len;
>  
>      if (use_gdb_syscalls()) {
> @@ -82,3 +80,21 @@ int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len)
>      g_string_free(s, true);
>      return out;
>  }
> +
> +void qemu_semihosting_console_outc(CPUArchState *env, target_ulong addr)
> +{
> +    CPUState *cpu = ENV_GET_CPU(env);
> +    uint8_t c;
> +
> +    if (cpu_memory_rw_debug(cpu, addr, &c, 1, 0) == 0) {
> +        if (use_gdb_syscalls()) {
> +            gdb_do_syscall(semihosting_cb, "write,2,%x,%x", addr, 1);
> +        } else {
> +            qemu_semihosting_log_out((const char *) &c, 1);
> +        }
> +    } else {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: passed inaccessible address " TARGET_FMT_lx,
> +                      __func__, addr);
> +    }
> +}
> diff --git a/include/hw/semihosting/console.h b/include/hw/semihosting/console.h
> index 30e66ae20aa..3a4fba75905 100644
> --- a/include/hw/semihosting/console.h
> +++ b/include/hw/semihosting/console.h
> @@ -10,17 +10,30 @@
>  #define _SEMIHOST_CONSOLE_H_
>  
>  /**
> - * qemu_semihosting_console_out:
> + * qemu_semihosting_console_outs:
>   * @env: CPUArchState
> - * @s: host address of guest string
> - * @len: length of string or 0 (string is null terminated)
> + * @s: host address of null terminated guest string
>   *
> - * Send a guest string to the debug console. This may be the remote
> - * gdb session if a softmmu guest is currently being debugged.
> + * Send a null terminated guest string to the debug console. This may
> + * be the remote gdb session if a softmmu guest is currently being
> + * debugged.
>   *
>   * Returns: number of bytes written.
>   */
> -int qemu_semihosting_console_out(CPUArchState *env, target_ulong s, int len);
> +int qemu_semihosting_console_outs(CPUArchState *env, target_ulong s);
> +
> +/**
> + * qemu_semihosting_console_outc:
> + * @env: CPUArchState
> + * @s: host address of null terminated guest string
> + *
> + * Send single character from guest memory to the debug console. This
> + * may be the remote gdb session if a softmmu guest is currently being
> + * debugged.
> + *
> + * Returns: nothing
> + */
> +void qemu_semihosting_console_outc(CPUArchState *env, target_ulong c);
>  
>  /**
>   * qemu_semihosting_log_out:
> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
> index 9554102a855..b7cdc21f832 100644
> --- a/linux-user/arm/semihost.c
> +++ b/linux-user/arm/semihost.c
> @@ -15,10 +15,34 @@
>  #include "hw/semihosting/console.h"
>  #include "qemu.h"
>  
> -int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len)
> +int qemu_semihosting_console_outs(CPUArchState *env, target_ulong addr)
>  {
> +    int len;
>      void *s = lock_user_string(addr);
> -    len = write(STDERR_FILENO, s, len ? len : strlen(s));
> +    if (!s) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: passed inaccessible address " TARGET_FMT_lx,
> +                      __func__, addr);
> +        return 0;
> +    }
> +
> +    len = write(STDERR_FILENO, s, strlen(s));

You could avoid 2 calls to strlen() if you inline directly
lock_user_string() content:

    len = target_strlen(addr);
    if (len < 0){
       qemu_log_mask(LOG_GUEST_ERROR,
                     "%s: passed inaccessible address " TARGET_FMT_lx,
                     __func__, addr);
       return 0;
    }
    s = lock_user(VERIFY_READ, addr, (long)(len + 1), 1);
    len = write(STDERR_FILENO, s, len);

>      unlock_user(s, addr, 0);
>      return len;
>  }

Thanks,
Laurent

Re: [Qemu-devel] [RFC PATCH] semihosting: split console_out intro string and char versions

[Alex Bennée]

Laurent Vivier <laurent@vivier.eu> writes:

> Le 30/05/2019 à 16:39, Alex Bennée a écrit :
>> This is ostensibly to avoid the weirdness of len looking like it might
>> come from a guest and sometimes being used. While we are at it fix up
>> the error checking for the arm-linux-user implementation of the API
>> which got flagged up by Coverity (CID 1401700).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
<snip>
>>  /**
>>   * qemu_semihosting_log_out:
>> diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c
>> index 9554102a855..b7cdc21f832 100644
>> --- a/linux-user/arm/semihost.c
>> +++ b/linux-user/arm/semihost.c
>> @@ -15,10 +15,34 @@
>>  #include "hw/semihosting/console.h"
>>  #include "qemu.h"
>>
>> -int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len)
>> +int qemu_semihosting_console_outs(CPUArchState *env, target_ulong addr)
>>  {
>> +    int len;
>>      void *s = lock_user_string(addr);
>> -    len = write(STDERR_FILENO, s, len ? len : strlen(s));
>> +    if (!s) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: passed inaccessible address " TARGET_FMT_lx,
>> +                      __func__, addr);
>> +        return 0;
>> +    }
>> +
>> +    len = write(STDERR_FILENO, s, strlen(s));
>
> You could avoid 2 calls to strlen() if you inline directly
> lock_user_string() content:
>
>     len = target_strlen(addr);
>     if (len < 0){
>        qemu_log_mask(LOG_GUEST_ERROR,
>                      "%s: passed inaccessible address " TARGET_FMT_lx,
>                      __func__, addr);
>        return 0;
>     }
>     s = lock_user(VERIFY_READ, addr, (long)(len + 1), 1);
>     len = write(STDERR_FILENO, s, len);
>
>>      unlock_user(s, addr, 0);
>>      return len;
>>  }

It's a nice clean-up but I've just looked at what was going on inside
with lock_user. I guess g_assert(s) on the lock user would be fair as we
can't fail at this point?

--
Alex Bennée