[Libguestfs] [PATCH libnbd v2 1/2] lib: Don't use perror after fork in nbd_connect_callback.
Eric Blake
eblake at redhat.com
Mon Sep 30 21:27:15 UTC 2019
On 9/30/19 11:32 AM, Richard W.M. Jones wrote:
> perror is not fork-safe and so could deadlock. Instead open code a
> fork-safe version of perror. While this fixes the current behaviour,
> in the long term we'd like to capture the error message into the usual
> error mechanism, so this is not the full and final fix for this issue.
>
> Also this fixes the exit code to be 126/127 instead of 1.
>
> Thanks: Eric Blake
> ---
> TODO | 1 +
> configure.ac | 10 ++++++
> generator/states-connect.c | 7 ++--
> lib/internal.h | 2 ++
> lib/utils.c | 66 ++++++++++++++++++++++++++++++++++++++
> 5 files changed, 84 insertions(+), 2 deletions(-)
>
> +++ b/configure.ac
> @@ -77,6 +77,16 @@ AC_CHECK_HEADERS([\
> stdatomic.h \
> sys/endian.h])
>
> +dnl Check for sys_errlist (optional).
> +AC_MSG_CHECKING([for sys_errlist])
> +AC_TRY_LINK([], [extern int sys_errlist; char *p = &sys_errlist;], [
We probably ought to use AC_CACHE_CHECK for our various configure.ac
things, to let a user have more control at overriding things if we
probed wrong without having to patch configure.ac. But that's a
separate problem, not affecting this one.
> +
> +/* Like sprintf (s, "%ld", v). The caller must supply a scratch
> + * buffer which is large enough for the result (32 bytes is fine), but
> + * note that the returned string does not point to the start of this
> + * buffer.
> + */
> +const char *
> +nbd_internal_fork_safe_itoa (long v, char *buf, size_t bufsize)
> +{
> + size_t i = bufsize - 1;
> + bool neg = false;
> +
> + buf[i--] = '\0';
> + if (v < 0) {
> + neg = true;
> + v = -v; /* XXX fails for INT_MIN */
Easy enough to work around. In the parameters:
unsigned long v,
and the condition here should be:
if ((long) v < 0)
> + }
> + if (v == 0)
> + buf[i--] = '0';
> + else {
> + while (v) {
> + buf[i--] = '0' + (v % 10);
> + v /= 10;
> + }
> + }
> + if (neg)
> + buf[i--] = '-';
> +
> + i++;
> + return &buf[i];
Do we want to assert that bufsize is large enough? (Or rather, abort()
if it is not, since assert() is borderline in fork-safe context)
> +}
> +
> +/* Fork-safe version of perror. ONLY use this after fork and before
> + * exec, the rest of the time use set_error().
> + */
> +void
> +nbd_internal_fork_safe_perror (const char *s)
> +{
> + const int err = errno;
> +
> +#if defined(__GNUC__)
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wunused-result"
> +#endif
> + write (2, s, strlen (s));
Surprisingly, strlen() is not listed in current POSIX' list of
async-signal-safe functions. But I have an open bug to remedy that, and
don't see any problem in using it.
I think it looks nicer to use STDERR_FILENO instead of 2.
> + write (2, ": ", 2);
> +#if HAVE_SYS_ERRLIST
> + write (2, sys_errlist[errno], strlen (sys_errlist[errno]));
> +#else
> + char buf[32];
> + const char *v = nbd_internal_fork_safe_itoa ((long) errno, buf, sizeof buf);
> + write (2, v, strlen (v));
> +#endif
> + write (2, "\n", 1);
> +#if defined(__GNUC__)
> +#pragma GCC diagnostic pop
> +#endif
> +
> + /* Restore original errno in case it was disturbed by the system
> + * calls above.
> + */
> + errno = err;
> +}
>
Otherwise looks sane.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
More information about the Libguestfs
mailing list