[RFC PATCH v2 1/2] util: Add thread-safe qemu_strerror() function

[RFC PATCH v2 1/2] util: Add thread-safe qemu_strerror() function

[Yohei Kojima]

Add qemu_strerror() which follows the POSIX specification for
strerror(). While strerror() is not guaranteed to be thread-safe, this
function is thread-safe.

This function is added to solve the following issue:
https://gitlab.com/qemu-project/qemu/-/issues/416

Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
---
 include/qemu/cutils.h | 20 +++++++++++++++++++
 util/cutils.c         | 45 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 92c436d8c7..0bcae0049a 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -117,6 +117,26 @@ int stristart(const char *str, const char *val, const char **ptr);
  * Returns: length of @s in bytes, or @max_len, whichever is smaller.
  */
 int qemu_strnlen(const char *s, int max_len);
+/**
+ * qemu_strerror:
+ * @errnum: error number
+ *
+ * Return the pointer to a string that describes errnum, like
+ * strerror(). This function is thread-safe because the buffer for
+ * returned string is allocated per thread.
+ *
+ * This function is thread-safe, but not reentrant. In other words,
+ * if a thread is interrupted by a signal in this function, and the
+ * thread calls this function again in the signal handling, then
+ * the result might be corrupted.
+ *
+ * This function has the same behaviour as the POSIX strerror()
+ * function.
+ *
+ * Returns: the pointer to an error description, or an
+ * "Unknown error nnn" message if errnum is invalid.
+ */
+char *qemu_strerror(int errnum);
 /**
  * qemu_strsep:
  * @input: pointer to string to parse
diff --git a/util/cutils.c b/util/cutils.c
index 5887e74414..3d14f50c75 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -131,6 +131,51 @@ int qemu_strnlen(const char *s, int max_len)
     return i;
 }
 
+/**
+ * It assumes the length of error descriptions are at most 1024.
+ * The concern of write buffer overflow is cared by strerror_r().
+ */
+static __thread char qemu_strerror_buf[1024];
+
+#if (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE
+/**
+ * In POSIX.1-2001 and after, the return type of strerror_r is int, but
+ * glibc overrides the definition of strerror_r to the old strerror_r
+ * if _GNU_SOURCE is defined. This condition handles it.
+ */
+
+char *qemu_strerror(int errnum)
+{
+    int is_error = strerror_r(errnum, qemu_strerror_buf, 1024);
+
+    if (is_error == 0) return qemu_strerror_buf;
+
+    /**
+     * handle the error occured in strerror_r
+     *
+     * If is_error is greater than 0, errno might not be updated by
+     * strerror_r. Otherwise, errno is updated.
+     */
+    if (is_error > 0) errno = is_error;
+
+    strncpy(qemu_strerror_buf, "Error %d occured\n", errno);
+    return qemu_strerror_buf;
+}
+#else
+/**
+ * In glibc, the return type of strerror_r is char* if _GNU_SOURCE
+ * is defined. In this case, strerror_r returns qemu_strerror_buf iff
+ * some error occured in strerror_r, and otherwise it returns a pointer
+ * to the pre-defined description for errnum.
+ *
+ * This is the same behaviour until POSIX.1-2001.
+ */
+char *qemu_strerror(int errnum)
+{
+    return strerror_r(errnum, qemu_strerror_buf, 1024);
+}
+#endif
+
 char *qemu_strsep(char **input, const char *delim)
 {
     char *result = *input;
-- 
2.39.2

[RFC PATCH v2 2/2] linux-user: replace strerror() function to the thread safe qemu_strerror()

[Yohei Kojima]

strerror() is not guaranteed to be thread-safe as described in
(https://gitlab.com/qemu-project/qemu/-/issues/416).

This commit changes files under /linux-user that call strerror() to call
the safer qemu_strerror().

Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
---
 linux-user/elfload.c | 4 ++--
 linux-user/main.c    | 4 ++--
 linux-user/syscall.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 150d1d4503..6ed2141674 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2771,7 +2771,7 @@ static void pgb_reserved_va(const char *image_name, abi_ulong guest_loaddr,
         error_report("Unable to reserve 0x%lx bytes of virtual address "
                      "space at %p (%s) for use as guest address space (check your "
                      "virtual memory ulimit setting, min_mmap_addr or reserve less "
-                     "using -R option)", reserved_va, test, strerror(errno));
+                     "using -R option)", reserved_va, test, qemu_strerror(errno));
         exit(EXIT_FAILURE);
     }
 
@@ -3564,7 +3564,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
     g_free(scratch);
 
     if (!bprm->p) {
-        fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG));
+        fprintf(stderr, "%s: %s\n", bprm->filename, qemu_strerror(E2BIG));
         exit(-1);
     }
 
diff --git a/linux-user/main.c b/linux-user/main.c
index 4b18461969..e0e133d631 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -744,7 +744,7 @@ int main(int argc, char **argv, char **envp)
     if (execfd == 0) {
         execfd = open(exec_path, O_RDONLY);
         if (execfd < 0) {
-            printf("Error while loading %s: %s\n", exec_path, strerror(errno));
+            printf("Error while loading %s: %s\n", exec_path, qemu_strerror(errno));
             _exit(EXIT_FAILURE);
         }
     }
@@ -887,7 +887,7 @@ int main(int argc, char **argv, char **envp)
     ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
         info, &bprm);
     if (ret != 0) {
-        printf("Error while loading %s: %s\n", exec_path, strerror(-ret));
+        printf("Error while loading %s: %s\n", exec_path, qemu_strerror(-ret));
         _exit(EXIT_FAILURE);
     }
 
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 24cea6fb6a..50090feac5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -580,7 +580,7 @@ const char *target_strerror(int err)
         return "Successful exit from sigreturn";
     }
 
-    return strerror(target_to_host_errno(err));
+    return qemu_strerror(target_to_host_errno(err));
 }
 
 static int check_zeroed_user(abi_long addr, size_t ksize, size_t usize)
-- 
2.39.2

Re: [RFC PATCH v2 1/2] util: Add thread-safe qemu_strerror() function

[Yohei Kojima]

I'm sorry for sending ill-formed thread twice.
This problem was because the SMTP server overwrites Message-ID,
and git-sendemail does not reflect it to In-Reply-To: and Reply-To: in the header.
I will test well before sending the next patch.

The original cover letter was
https://lore.kernel.org/qemu-devel/TYZPR06MB5418216269D0ED2EB37D6FF49DBE9@TYZPR06MB5418.apcprd06.prod.outlook.com/T/#u

Thank you.

On 2023/03/14 15:40, Yohei Kojima wrote:
> Add qemu_strerror() which follows the POSIX specification for
> strerror(). While strerror() is not guaranteed to be thread-safe, this
> function is thread-safe.
> 
> This function is added to solve the following issue:
> https://gitlab.com/qemu-project/qemu/-/issues/416
> 
> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
> ---
>  include/qemu/cutils.h | 20 +++++++++++++++++++
>  util/cutils.c         | 45 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 92c436d8c7..0bcae0049a 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -117,6 +117,26 @@ int stristart(const char *str, const char *val, const char **ptr);
>   * Returns: length of @s in bytes, or @max_len, whichever is smaller.
>   */
>  int qemu_strnlen(const char *s, int max_len);
> +/**
> + * qemu_strerror:
> + * @errnum: error number
> + *
> + * Return the pointer to a string that describes errnum, like
> + * strerror(). This function is thread-safe because the buffer for
> + * returned string is allocated per thread.
> + *
> + * This function is thread-safe, but not reentrant. In other words,
> + * if a thread is interrupted by a signal in this function, and the
> + * thread calls this function again in the signal handling, then
> + * the result might be corrupted.
> + *
> + * This function has the same behaviour as the POSIX strerror()
> + * function.
> + *
> + * Returns: the pointer to an error description, or an
> + * "Unknown error nnn" message if errnum is invalid.
> + */
> +char *qemu_strerror(int errnum);
>  /**
>   * qemu_strsep:
>   * @input: pointer to string to parse
> diff --git a/util/cutils.c b/util/cutils.c
> index 5887e74414..3d14f50c75 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -131,6 +131,51 @@ int qemu_strnlen(const char *s, int max_len)
>      return i;
>  }
>  
> +/**
> + * It assumes the length of error descriptions are at most 1024.
> + * The concern of write buffer overflow is cared by strerror_r().
> + */
> +static __thread char qemu_strerror_buf[1024];
> +
> +#if (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE
> +/**
> + * In POSIX.1-2001 and after, the return type of strerror_r is int, but
> + * glibc overrides the definition of strerror_r to the old strerror_r
> + * if _GNU_SOURCE is defined. This condition handles it.
> + */
> +
> +char *qemu_strerror(int errnum)
> +{
> +    int is_error = strerror_r(errnum, qemu_strerror_buf, 1024);
> +
> +    if (is_error == 0) return qemu_strerror_buf;
> +
> +    /**
> +     * handle the error occured in strerror_r
> +     *
> +     * If is_error is greater than 0, errno might not be updated by
> +     * strerror_r. Otherwise, errno is updated.
> +     */
> +    if (is_error > 0) errno = is_error;
> +
> +    strncpy(qemu_strerror_buf, "Error %d occured\n", errno);
> +    return qemu_strerror_buf;
> +}
> +#else
> +/**
> + * In glibc, the return type of strerror_r is char* if _GNU_SOURCE
> + * is defined. In this case, strerror_r returns qemu_strerror_buf iff
> + * some error occured in strerror_r, and otherwise it returns a pointer
> + * to the pre-defined description for errnum.
> + *
> + * This is the same behaviour until POSIX.1-2001.
> + */
> +char *qemu_strerror(int errnum)
> +{
> +    return strerror_r(errnum, qemu_strerror_buf, 1024);
> +}
> +#endif
> +
>  char *qemu_strsep(char **input, const char *delim)
>  {
>      char *result = *input;

Re: [RFC PATCH v2 1/2] util: Add thread-safe qemu_strerror() function

[Alex Bennée]

Yohei Kojima <y-koj@outlook.jp> writes:

> Add qemu_strerror() which follows the POSIX specification for
> strerror(). While strerror() is not guaranteed to be thread-safe, this
> function is thread-safe.
>
> This function is added to solve the following issue:
> https://gitlab.com/qemu-project/qemu/-/issues/416

If you tag this as:

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/416

then it will auto clear when merged.

>
> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
> ---
>  include/qemu/cutils.h | 20 +++++++++++++++++++
>  util/cutils.c         | 45 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 92c436d8c7..0bcae0049a 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -117,6 +117,26 @@ int stristart(const char *str, const char *val, const char **ptr);
>   * Returns: length of @s in bytes, or @max_len, whichever is smaller.
>   */
>  int qemu_strnlen(const char *s, int max_len);
> +/**
> + * qemu_strerror:
> + * @errnum: error number
> + *
> + * Return the pointer to a string that describes errnum, like
> + * strerror(). This function is thread-safe because the buffer for
> + * returned string is allocated per thread.
> + *
> + * This function is thread-safe, but not reentrant. In other words,
> + * if a thread is interrupted by a signal in this function, and the
> + * thread calls this function again in the signal handling, then
> + * the result might be corrupted.
> + *
> + * This function has the same behaviour as the POSIX strerror()
> + * function.
> + *
> + * Returns: the pointer to an error description, or an
> + * "Unknown error nnn" message if errnum is invalid.
> + */
> +char *qemu_strerror(int errnum);
>  /**
>   * qemu_strsep:
>   * @input: pointer to string to parse
> diff --git a/util/cutils.c b/util/cutils.c
> index 5887e74414..3d14f50c75 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -131,6 +131,51 @@ int qemu_strnlen(const char *s, int max_len)
>      return i;
>  }
>  
> +/**
> + * It assumes the length of error descriptions are at most 1024.
> + * The concern of write buffer overflow is cared by strerror_r().
> + */
> +static __thread char qemu_strerror_buf[1024];
> +
> +#if (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE
> +/**
> + * In POSIX.1-2001 and after, the return type of strerror_r is int, but
> + * glibc overrides the definition of strerror_r to the old strerror_r
> + * if _GNU_SOURCE is defined. This condition handles it.
> + */
> +
> +char *qemu_strerror(int errnum)
> +{
> +    int is_error = strerror_r(errnum, qemu_strerror_buf, 1024);
> +
> +    if (is_error == 0) return qemu_strerror_buf;
> +
> +    /**
> +     * handle the error occured in strerror_r
> +     *
> +     * If is_error is greater than 0, errno might not be updated by
> +     * strerror_r. Otherwise, errno is updated.
> +     */
> +    if (is_error > 0) errno = is_error;
> +
> +    strncpy(qemu_strerror_buf, "Error %d occured\n", errno);
> +    return qemu_strerror_buf;
> +}
> +#else
> +/**
> + * In glibc, the return type of strerror_r is char* if _GNU_SOURCE
> + * is defined. In this case, strerror_r returns qemu_strerror_buf iff
> + * some error occured in strerror_r, and otherwise it returns a pointer
> + * to the pre-defined description for errnum.
> + *
> + * This is the same behaviour until POSIX.1-2001.
> + */
> +char *qemu_strerror(int errnum)
> +{
> +    return strerror_r(errnum, qemu_strerror_buf, 1024);
> +}
> +#endif
> +
>  char *qemu_strsep(char **input, const char *delim)
>  {
>      char *result = *input;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v2 1/2] util: Add thread-safe qemu_strerror() function

[Alex Bennée]

Yohei Kojima <y-koj@outlook.jp> writes:

> Add qemu_strerror() which follows the POSIX specification for
> strerror(). While strerror() is not guaranteed to be thread-safe, this
> function is thread-safe.
>
> This function is added to solve the following issue:
> https://gitlab.com/qemu-project/qemu/-/issues/416
>
> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
> ---
>  include/qemu/cutils.h | 20 +++++++++++++++++++
>  util/cutils.c         | 45 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 92c436d8c7..0bcae0049a 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -117,6 +117,26 @@ int stristart(const char *str, const char *val, const char **ptr);
>   * Returns: length of @s in bytes, or @max_len, whichever is smaller.
>   */
>  int qemu_strnlen(const char *s, int max_len);
> +/**
> + * qemu_strerror:
> + * @errnum: error number
> + *
> + * Return the pointer to a string that describes errnum, like
> + * strerror(). This function is thread-safe because the buffer for
> + * returned string is allocated per thread.
> + *
> + * This function is thread-safe, but not reentrant. In other words,
> + * if a thread is interrupted by a signal in this function, and the
> + * thread calls this function again in the signal handling, then
> + * the result might be corrupted.
> + *
> + * This function has the same behaviour as the POSIX strerror()
> + * function.
> + *
> + * Returns: the pointer to an error description, or an
> + * "Unknown error nnn" message if errnum is invalid.
> + */
> +char *qemu_strerror(int errnum);
>  /**
>   * qemu_strsep:
>   * @input: pointer to string to parse
> diff --git a/util/cutils.c b/util/cutils.c
> index 5887e74414..3d14f50c75 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -131,6 +131,51 @@ int qemu_strnlen(const char *s, int max_len)
>      return i;
>  }
>  
> +/**
> + * It assumes the length of error descriptions are at most 1024.
> + * The concern of write buffer overflow is cared by strerror_r().
> + */
> +static __thread char qemu_strerror_buf[1024];
> +
> +#if (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE
> +/**
> + * In POSIX.1-2001 and after, the return type of strerror_r is int, but
> + * glibc overrides the definition of strerror_r to the old strerror_r
> + * if _GNU_SOURCE is defined. This condition handles it.
> + */
> +
> +char *qemu_strerror(int errnum)
> +{
> +    int is_error = strerror_r(errnum, qemu_strerror_buf, 1024);
> +
> +    if (is_error == 0) return qemu_strerror_buf;

Please follow coding style rules. Checkpatch would have complained here.

> +
> +    /**
> +     * handle the error occured in strerror_r
> +     *
> +     * If is_error is greater than 0, errno might not be updated by
> +     * strerror_r. Otherwise, errno is updated.
> +     */
> +    if (is_error > 0) errno = is_error;

and here.

> +
> +    strncpy(qemu_strerror_buf, "Error %d occured\n", errno);
> +    return qemu_strerror_buf;
> +}
> +#else
> +/**
> + * In glibc, the return type of strerror_r is char* if _GNU_SOURCE
> + * is defined. In this case, strerror_r returns qemu_strerror_buf iff
> + * some error occured in strerror_r, and otherwise it returns a pointer
> + * to the pre-defined description for errnum.
> + *
> + * This is the same behaviour until POSIX.1-2001.
> + */
> +char *qemu_strerror(int errnum)
> +{
> +    return strerror_r(errnum, qemu_strerror_buf, 1024);
> +}
> +#endif
> +
>  char *qemu_strsep(char **input, const char *delim)
>  {
>      char *result = *input;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v2 2/2] linux-user: replace strerror() function to the thread safe qemu_strerror()

[Alex Bennée]

Yohei Kojima <y-koj@outlook.jp> writes:

> strerror() is not guaranteed to be thread-safe as described in
> (https://gitlab.com/qemu-project/qemu/-/issues/416).
>
> This commit changes files under /linux-user that call strerror() to call
> the safer qemu_strerror().

As mentioned on bug tracker I think you need some more patches for the
other uses. But for this one:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v2 2/2] linux-user: replace strerror() function to the thread safe qemu_strerror()

[Yohei Kojima]

On 2023/03/30 23:08, Alex Bennée wrote:
> 
> Yohei Kojima <y-koj@outlook.jp> writes:
> 
>> strerror() is not guaranteed to be thread-safe as described in
>> (https://gitlab.com/qemu-project/qemu/-/issues/416).
>>
>> This commit changes files under /linux-user that call strerror() to call
>> the safer qemu_strerror().
> 
> As mentioned on bug tracker I think you need some more patches for the
> other uses. But for this one:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 

Thank you for the review. I will add several patches for other subsystems.