[PATCH common v2] utils: Fix usage of strerror_r
Slightly modified version of the previous fix, but essentially the same thing. I have put the change into common/utils/utils.c so that I could use the same code to fix the Lua bindings which were also calling strerror_r incorrectly. Rich.
$ ./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. Provide an easier to use wrapper around strerror_r and try to cope with the two variants using _GNU_SOURCE. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2030396 Reported-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- utils/guestfs-utils.h | 1 + utils/utils.c | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/utils/guestfs-utils.h b/utils/guestfs-utils.h index b5f5f3ca4..b1d923e0e 100644 --- a/utils/guestfs-utils.h +++ b/utils/guestfs-utils.h @@ -67,6 +67,7 @@ extern int guestfs_int_is_lnk (int64_t mode); extern int guestfs_int_is_sock (int64_t mode); extern char *guestfs_int_full_path (const char *dir, const char *name); extern void guestfs_int_hexdump (const void *data, size_t len, FILE *fp); +const char *guestfs_int_strerror (int errnum, char *buf, size_t buflen); /* Not all language bindings know how to deal with Pointer arguments. * Those that don't will use this macro which complains noisily and diff --git a/utils/utils.c b/utils/utils.c index 6f4108732..70e55cb2e 100644 --- a/utils/utils.c +++ b/utils/utils.c @@ -640,3 +640,28 @@ guestfs_int_hexdump (const void *data, size_t len, FILE *fp) fprintf (fp, "|\n"); } } + +/** + * Thread-safe strerror_r. + * + * This is a wrapper around the two variants of L<strerror_r(3)> + * in glibc since it is hard to use correctly (RHBZ#2030396). + * + * The buffer passed in should be large enough to store the + * error message (256 chars at least) and should be non-static. + * Note that the buffer might not be used, use the return value. + */ +const char * +guestfs_int_strerror (int errnum, char *buf, size_t buflen) +{ +#ifdef _GNU_SOURCE + /* GNU strerror_r */ + return strerror_r (errnum, buf, buflen); +#else + /* XSI-compliant strerror_r */ + int err = strerror_r (errnum, buf, buflen); + if (err > 0) + snprintf (buf, buflen, "error number %d", errnum); + return buf; +#endif +} -- 2.32.0
On Thu, Dec 09, 2021 at 09:05:45AM +0000, Richard W.M. Jones wrote:
diff --git a/utils/guestfs-utils.h b/utils/guestfs-utils.h index b5f5f3ca4..b1d923e0e 100644 --- a/utils/guestfs-utils.h +++ b/utils/guestfs-utils.h @@ -67,6 +67,7 @@ extern int guestfs_int_is_lnk (int64_t mode); extern int guestfs_int_is_sock (int64_t mode); extern char *guestfs_int_full_path (const char *dir, const char *name); extern void guestfs_int_hexdump (const void *data, size_t len, FILE *fp); +const char *guestfs_int_strerror (int errnum, char *buf, size_t buflen);
That should be extern. I wonder why that didn't give an error? Anyway, fixed in my local copy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 12/09/21 10:09, Richard W.M. Jones wrote:
On Thu, Dec 09, 2021 at 09:05:45AM +0000, Richard W.M. Jones wrote:
diff --git a/utils/guestfs-utils.h b/utils/guestfs-utils.h index b5f5f3ca4..b1d923e0e 100644 --- a/utils/guestfs-utils.h +++ b/utils/guestfs-utils.h @@ -67,6 +67,7 @@ extern int guestfs_int_is_lnk (int64_t mode); extern int guestfs_int_is_sock (int64_t mode); extern char *guestfs_int_full_path (const char *dir, const char *name); extern void guestfs_int_hexdump (const void *data, size_t len, FILE *fp); +const char *guestfs_int_strerror (int errnum, char *buf, size_t buflen);
That should be extern. I wonder why that didn't give an error?
Because it's not required. ISO C99, "6.2.2 Linkages of identifiers", paragraph 5: If the declaration of an identifier for a function has no storage-class specifier, its linkage is determined exactly as if it were declared with the storage-class specifier extern. [...]
Anyway, fixed in my local copy.
I do agree it's better to be consistent. Laszlo
On 12/09/21 10:05, 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.
Provide an easier to use wrapper around strerror_r and try to cope with the two variants using _GNU_SOURCE.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2030396 Reported-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- utils/guestfs-utils.h | 1 + utils/utils.c | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+)
diff --git a/utils/guestfs-utils.h b/utils/guestfs-utils.h index b5f5f3ca4..b1d923e0e 100644 --- a/utils/guestfs-utils.h +++ b/utils/guestfs-utils.h @@ -67,6 +67,7 @@ extern int guestfs_int_is_lnk (int64_t mode); extern int guestfs_int_is_sock (int64_t mode); extern char *guestfs_int_full_path (const char *dir, const char *name); extern void guestfs_int_hexdump (const void *data, size_t len, FILE *fp); +const char *guestfs_int_strerror (int errnum, char *buf, size_t buflen);
/* Not all language bindings know how to deal with Pointer arguments. * Those that don't will use this macro which complains noisily and diff --git a/utils/utils.c b/utils/utils.c index 6f4108732..70e55cb2e 100644 --- a/utils/utils.c +++ b/utils/utils.c @@ -640,3 +640,28 @@ guestfs_int_hexdump (const void *data, size_t len, FILE *fp) fprintf (fp, "|\n"); } } + +/** + * Thread-safe strerror_r. + * + * This is a wrapper around the two variants of L<strerror_r(3)> + * in glibc since it is hard to use correctly (RHBZ#2030396). + * + * The buffer passed in should be large enough to store the + * error message (256 chars at least) and should be non-static. + * Note that the buffer might not be used, use the return value. + */ +const char * +guestfs_int_strerror (int errnum, char *buf, size_t buflen) +{ +#ifdef _GNU_SOURCE + /* GNU strerror_r */ + return strerror_r (errnum, buf, buflen); +#else + /* XSI-compliant strerror_r */ + int err = strerror_r (errnum, buf, buflen); + if (err > 0) + snprintf (buf, buflen, "error number %d", errnum); + return buf; +#endif +}
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones