thr3ads.net - Libguestfs - [Libguestfs] [PATCH] lib/errors.c: Fix usage of strerror

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2021-Dec-08 17:06 UTC

[Libguestfs] [PATCH] lib/errors.c: Fix usage of strerror_r

$ ./run guestfish -vx add-drive foo "readonly:true"
  libguestfs: trace: set_pgroup true
  libguestfs: trace: set_pgroup = 0
  libguestfs: trace: add_drive "foo" "readonly:true"
  libguestfs: error: foo:
  libguestfs: trace: add_drive = -1 (error)
  libguestfs: trace: close
  libguestfs: closing guestfs handle 0x55c0bacf12a0 (state 0)

Note the "error: foo: " line.  After hexdumping this I found that it
is actually printing random rubbish after the colon.  It turns out
that strerror_r was not updating the buffer and so we were printing
random bytes from the stack.

strerror_r is hard to use and worse still there are two incompatible
variants in glibc.  Close reading of the GNU variant that we are using
shows that it doesn't always use the buffer passed in, which explains
what was happening here.

Try to fix usage of strerror_r and try to cope with the two variants
using _GNU_SOURCE.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2030396
---
 lib/errors.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lib/errors.c b/lib/errors.c
index bd9699eeb1..d599a21879 100644
--- a/lib/errors.c
+++ b/lib/errors.c
@@ -322,6 +322,7 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
   const int errnum = errno;
   int err;
   char buf[256];
+  const char *errstr;
   struct error_data *error_data = get_error_data (g);
 
   va_start (args, fs);
@@ -330,11 +331,19 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
 
   if (err < 0) return;
 
-  ignore_value (strerror_r (errnum, buf, sizeof buf));
+#ifdef _GNU_SOURCE
+  /* GNU strerror_r */
+  errstr = strerror_r (errnum, buf, sizeof buf);
+#else
+  /* XSI-compliant strerror_r */
+  err = strerror_r (errnum, buf, sizeof buf);
+  if (err > 0) return;
+  errstr = buf;
+#endif
 
-  msg = safe_realloc (g, msg, strlen (msg) + 2 + strlen (buf) + 1);
+  msg = safe_realloc (g, msg, strlen (msg) + 2 + strlen (errstr) + 1);
   strcat (msg, ": ");
-  strcat (msg, buf);
+  strcat (msg, errstr);
 
   /* set_last_error first so that the callback can access the error
    * message and errno through the handle if it wishes.
-- 
2.32.0

Masayoshi Mizuma

2021-Dec-08 23:26 UTC

head link

[Libguestfs] [PATCH] lib/errors.c: Fix usage of strerror_r

On Wed, Dec 08, 2021 at 05:06:24PM +0000, Richard W.M. Jones
wrote:>   $ ./run guestfish -vx add-drive foo "readonly:true"
>   libguestfs: trace: set_pgroup true
>   libguestfs: trace: set_pgroup = 0
>   libguestfs: trace: add_drive "foo" "readonly:true"
>   libguestfs: error: foo:
>   libguestfs: trace: add_drive = -1 (error)
>   libguestfs: trace: close
>   libguestfs: closing guestfs handle 0x55c0bacf12a0 (state 0)
> 
> Note the "error: foo: " line.  After hexdumping this I found that
it
> is actually printing random rubbish after the colon.  It turns out
> that strerror_r was not updating the buffer and so we were printing
> random bytes from the stack.
> 
> strerror_r is hard to use and worse still there are two incompatible
> variants in glibc.  Close reading of the GNU variant that we are using
> shows that it doesn't always use the buffer passed in, which explains
> what was happening here.
> 
> Try to fix usage of strerror_r and try to cope with the two variants
> using _GNU_SOURCE.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2030396
Thank you for this patch. It works well!

  $ ./run guestfish -vx add-drive foo "readonly:true"
  libguestfs: trace: set_pgroup true
  libguestfs: trace: set_pgroup = 0
  libguestfs: trace: add_drive "foo" "readonly:true"
  libguestfs: error: foo: No such file or directory
  libguestfs: trace: add_drive = -1 (error)
  libguestfs: trace: close
  libguestfs: closing guestfs handle 0x103c130 (state 0)
  $

Please feel free to add:

Tested-by: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>

Thanks!
Masa
> ---
>  lib/errors.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/errors.c b/lib/errors.c
> index bd9699eeb1..d599a21879 100644
> --- a/lib/errors.c
> +++ b/lib/errors.c
> @@ -322,6 +322,7 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
>    const int errnum = errno;
>    int err;
>    char buf[256];
> +  const char *errstr;
>    struct error_data *error_data = get_error_data (g);
>  
>    va_start (args, fs);
> @@ -330,11 +331,19 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs,
...)
>  
>    if (err < 0) return;
>  
> -  ignore_value (strerror_r (errnum, buf, sizeof buf));
> +#ifdef _GNU_SOURCE
> +  /* GNU strerror_r */
> +  errstr = strerror_r (errnum, buf, sizeof buf);
> +#else
> +  /* XSI-compliant strerror_r */
> +  err = strerror_r (errnum, buf, sizeof buf);
> +  if (err > 0) return;
> +  errstr = buf;
> +#endif
>  
> -  msg = safe_realloc (g, msg, strlen (msg) + 2 + strlen (buf) + 1);
> +  msg = safe_realloc (g, msg, strlen (msg) + 2 + strlen (errstr) + 1);
>    strcat (msg, ": ");
> -  strcat (msg, buf);
> +  strcat (msg, errstr);
>  
>    /* set_last_error first so that the callback can access the error
>     * message and errno through the handle if it wishes.
> -- 
> 2.32.0

Laszlo Ersek

2021-Dec-09 13:11 UTC

head link

[Libguestfs] [PATCH] lib/errors.c: Fix usage of strerror_r

On 12/08/21 18:06, Richard W.M. Jones wrote:>   $ ./run guestfish -vx add-drive foo "readonly:true"
>   libguestfs: trace: set_pgroup true
>   libguestfs: trace: set_pgroup = 0
>   libguestfs: trace: add_drive "foo" "readonly:true"
>   libguestfs: error: foo:
>   libguestfs: trace: add_drive = -1 (error)
>   libguestfs: trace: close
>   libguestfs: closing guestfs handle 0x55c0bacf12a0 (state 0)
> 
> Note the "error: foo: " line.  After hexdumping this I found that
it
> is actually printing random rubbish after the colon.  It turns out
> that strerror_r was not updating the buffer and so we were printing
> random bytes from the stack.
> 
> strerror_r is hard to use and worse still there are two incompatible
> variants in glibc.  Close reading of the GNU variant that we are using
> shows that it doesn't always use the buffer passed in, which explains
> what was happening here.
> 
> Try to fix usage of strerror_r and try to cope with the two variants
> using _GNU_SOURCE.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2030396
> ---
>  lib/errors.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/errors.c b/lib/errors.c
> index bd9699eeb1..d599a21879 100644
> --- a/lib/errors.c
> +++ b/lib/errors.c
> @@ -322,6 +322,7 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs, ...)
>    const int errnum = errno;
>    int err;
>    char buf[256];
> +  const char *errstr;
>    struct error_data *error_data = get_error_data (g);
>  
>    va_start (args, fs);
> @@ -330,11 +331,19 @@ guestfs_int_perrorf (guestfs_h *g, const char *fs,
...)
>  
>    if (err < 0) return;
>  
> -  ignore_value (strerror_r (errnum, buf, sizeof buf));
> +#ifdef _GNU_SOURCE
> +  /* GNU strerror_r */
> +  errstr = strerror_r (errnum, buf, sizeof buf);
> +#else
> +  /* XSI-compliant strerror_r */
> +  err = strerror_r (errnum, buf, sizeof buf);
> +  if (err > 0) return;
> +  errstr = buf;
> +#endif
>  
> -  msg = safe_realloc (g, msg, strlen (msg) + 2 + strlen (buf) + 1);
> +  msg = safe_realloc (g, msg, strlen (msg) + 2 + strlen (errstr) + 1);
>    strcat (msg, ": ");
> -  strcat (msg, buf);
> +  strcat (msg, errstr);
>  
>    /* set_last_error first so that the callback can access the error
>     * message and errno through the handle if it wishes.
> 
Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Libguestfs - Dec 2021 - [PATCH] lib/errors.c: Fix usage of strerror_r

[Libguestfs] [PATCH] lib/errors.c: Fix usage of strerror_r

[Libguestfs] [PATCH] lib/errors.c: Fix usage of strerror_r

[Libguestfs] [PATCH] lib/errors.c: Fix usage of strerror_r