On 11/29/18 12:07 PM, Eric Blake wrote:> On 11/29/18 11:21 AM, Eric Blake wrote: >> Our use of "%m" in various error messages is testament to the >> project's initial life on Linux - but other than Cygwin, I know >> of no other platforms supporting that glibc extension. >> >> We COULD audit the code and manually turn "%m" into >> "%s"/strerror(errno), but that's a lot of churn. Instead, let's >> fix the few outliers that can't be easily wrapped, then wrap >> the remainder. >> >> While I was able to test this on Linux (both that no wrapper is >> used by default, and that faking that %m fails causes the wrapper >> to do the right thing), I haven't actually tried it on a BSD box, >> hence I'll wait for review before pushing. > > Not fixed here, but still worth doing: > > Audit and fix all our uses of nbdkit_error("...\n") to drop their > trailing newline, as nbdkit_error() adds one. Then update > nbdkit_error() to actually do smart newline appending (borrowing from > commit ef4f72ef) so that other callers outside our codebase get smart > handling by default.Also not fixed: strerror() is not threadsafe (at all) on FreeBSD. On glibc, it is at least thread-safe for safe input values (that is, if passed 0 or a value from <errno.h>, the resulting string has lifetime in const storage, even if it is mmap'd to a localized translation; and thus will not be corrupted by other threads' actions); but is not thread-safe for arbitrary values (such as strerror(-1), where glibc malloc's space shared between all threads for returning "Unknown error -1", and which could get corrupted by a parallel thread doing strerror(-2)). But FreeBSD shares static storage for strerror() results among all threads, in part because it computes the resulting string via catopen()/catgets()/catclose() and MUST copy the localized string somewhere because the source read via catgets() may not survive catclose(). So we really need to audit all use of sterror() in nbdkit and switch over to strerror_r(), remembering to work around the alternate glibc signature when _GNU_SOURCE is defined. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Jan-22 20:18 UTC
Re: [Libguestfs] [nbdkit PATCH 0/3] Fix %m usage on BSD
What about calls to gettext (ie. _(...))? gnutls_strerror seems at first glance to be thread safe, but it then passes the result through gettext. https://github.com/gnutls/gnutls/blob/5fb3a45e34a843942f0fe55d55779111d7f18eaa/lib/errors.c#L547 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 1/22/19 2:18 PM, Richard W.M. Jones wrote:> What about calls to gettext (ie. _(...))? gnutls_strerror seems at > first glance to be thread safe, but it then passes the result through > gettext. > > https://github.com/gnutls/gnutls/blob/5fb3a45e34a843942f0fe55d55779111d7f18eaa/lib/errors.c#L547I'm fairly certain that gettext() is thread-safe. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org