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.ben...@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
