[Libguestfs] [PATCH libnbd v2 1/2] lib: Don't use perror after fork in nbd_connect_callback.

[Eric Blake]

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