Libguestfs - Jul 2021 - [libnbd PATCH 3/6] macOS: Simple cloexec/nonblock fix

This is the most trivial way to fix the issue with macOS not having SOCK_CLOEXEC
and SOCK_NONBLOCK.  There is not much better way, so this is the only way to
make it work on such platform(s).

Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
---
 lib/internal.h                               |  3 +
 generator/states-connect-socket-activation.c |  2 +-
 generator/states-connect.c                   | 11 ++--
 lib/utils.c                                  | 68 ++++++++++++++++++++
 fuzzing/libnbd-fuzz-wrapper.c                | 30 ++++++++-
 fuzzing/libnbd-libfuzzer-test.c              | 30 ++++++++-
 6 files changed, 136 insertions(+), 8 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index 01f9d8ab5fea..8a4c189abe65 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -467,4 +467,7 @@ extern char *nbd_internal_printable_buffer (const void *buf,
size_t count);
 extern char *nbd_internal_printable_string (const char *str);
 extern char *nbd_internal_printable_string_list (char **list);
 
+extern int nbd_internal_socket(int domain, int type, int protocol, bool
nonblock);
+extern int nbd_internal_socketpair(int domain, int type, int protocol, int
*fds);
+
 #endif /* LIBNBD_INTERNAL_H */
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index e601c9bb56be..8a2add312bc4 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -131,7 +131,7 @@ STATE_MACHINE {
     return 0;
   }
 
-  s = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0);
+  s = nbd_internal_socket (AF_UNIX, SOCK_STREAM, 0, false);
   if (s == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socket");
diff --git a/generator/states-connect.c b/generator/states-connect.c
index fcac86f36a34..8de12183d627 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -52,7 +52,7 @@ STATE_MACHINE {
 
   assert (!h->sock);
   family = h->connaddr.ss_family;
-  fd = socket (family, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0);
+  fd = nbd_internal_socket (family, SOCK_STREAM, 0, true);
   if (fd == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socket");
@@ -162,9 +162,10 @@ STATE_MACHINE {
     return -1;
   }
 
-  fd = socket (h->rp->ai_family,
-               h->rp->ai_socktype|SOCK_NONBLOCK|SOCK_CLOEXEC,
-               h->rp->ai_protocol);
+  fd = nbd_internal_socket (h->rp->ai_family,
+                            h->rp->ai_socktype,
+                            h->rp->ai_protocol,
+                            true);
   if (fd == -1) {
     SET_NEXT_STATE (%NEXT_ADDRESS);
     return 0;
@@ -227,7 +228,7 @@ STATE_MACHINE {
   assert (!h->sock);
   assert (h->argv.ptr);
   assert (h->argv.ptr[0]);
-  if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) {
+  if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
     SET_NEXT_STATE (%.DEAD);
     set_error (errno, "socketpair");
     return 0;
diff --git a/lib/utils.c b/lib/utils.c
index 260fd6a25796..972cbc5208a7 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -24,6 +24,7 @@
 #include <unistd.h>
 #include <ctype.h>
 #include <errno.h>
+#include <fcntl.h>
 
 #include "minmax.h"
 
@@ -258,3 +259,70 @@ nbd_internal_printable_string_list (char **list)
   return s;
 
 }
+
+int nbd_internal_socket(int domain,
+                        int type,
+                        int protocol,
+                        bool nonblock)
+{
+  int fd;
+
+#ifdef __APPLE__
+  int flags;
+#else
+  type |= SOCK_CLOEXEC;
+  if (nonblock)
+    type |= SOCK_NONBLOCK;
+#endif
+
+  fd = socket (domain, type, protocol);
+
+#ifdef __APPLE__
+  if (fd == -1)
+    return -1;
+
+  if (fcntl (fd, F_SETFD, FD_CLOEXEC) == -1) {
+    close(fd);
+    return -1;
+  }
+
+  if (nonblock) {
+    flags = fcntl (fd, F_GETFL, 0);
+    if (flags == -1 ||
+        fcntl (fd, F_SETFL, flags|O_NONBLOCK) == -1) {
+      close(fd);
+      return -1;
+    }
+  }
+#endif
+
+  return fd;
+}
+
+int
+nbd_internal_socketpair (int domain, int type, int protocol, int *fds)
+{
+  int ret;
+
+#ifdef __APPLE__
+  size_t i;
+#else
+  type |= SOCK_CLOEXEC;
+#endif
+
+  ret = socketpair (domain, type, protocol, fds);
+
+#ifdef __APPLE__
+  if (ret == 0) {
+    for (i = 0; i < 2; i++) {
+      if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
+        close(fds[0]);
+        close(fds[1]);
+        return -1;
+      }
+    }
+  }
+#endif
+
+  return ret;
+}
diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c
index 99a6d803258f..3d127e673e9e 100644
--- a/fuzzing/libnbd-fuzz-wrapper.c
+++ b/fuzzing/libnbd-fuzz-wrapper.c
@@ -41,6 +41,34 @@
 static void client (int s);
 static void server (int fd, int s);
 
+static int
+get_socketpair (int domain, int type, int protocol, int *fds)
+{
+  int ret;
+
+#ifdef __APPLE__
+  size_t i;
+#else
+  type |= SOCK_CLOEXEC;
+#endif
+
+  ret = socketpair (domain, type, protocol, fds);
+
+#ifdef __APPLE__
+  if (ret == 0) {
+    for (i = 0; i < 2; i++) {
+      if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
+        close(fds[0]);
+        close(fds[1]);
+        return -1;
+      }
+    }
+  }
+#endif
+
+  return ret;
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -61,7 +89,7 @@ main (int argc, char *argv[])
   }
 
   /* Create a connected socket. */
-  if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) {
+  if (get_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
     perror ("socketpair");
     exit (EXIT_FAILURE);
   }
diff --git a/fuzzing/libnbd-libfuzzer-test.c b/fuzzing/libnbd-libfuzzer-test.c
index 5ee29b877bdb..0bf988ee8398 100644
--- a/fuzzing/libnbd-libfuzzer-test.c
+++ b/fuzzing/libnbd-libfuzzer-test.c
@@ -41,6 +41,34 @@
 static void client (int sock);
 static void server (const uint8_t *data, size_t size, int sock);
 
+static int
+get_socketpair (int domain, int type, int protocol, int *fds)
+{
+  int ret;
+
+#ifdef __APPLE__
+  size_t i;
+#else
+  type |= SOCK_CLOEXEC;
+#endif
+
+  ret = socketpair (domain, type, protocol, fds);
+
+#ifdef __APPLE__
+  if (ret == 0) {
+    for (i = 0; i < 2; i++) {
+      if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
+        close(fds[0]);
+        close(fds[1]);
+        return -1;
+      }
+    }
+  }
+#endif
+
+  return ret;
+}
+
 /* This is the entry point called by libFuzzer. */
 int
 LLVMFuzzerTestOneInput (const uint8_t *data, size_t size)
@@ -49,7 +77,7 @@ LLVMFuzzerTestOneInput (const uint8_t *data, size_t size)
   int sv[2], r, status;
 
   /* Create a connected socket. */
-  if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) {
+  if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1)
{
     perror ("socketpair");
     exit (EXIT_FAILURE);
   }
-- 
2.32.0

On Tue, Jul 13, 2021 at 11:26:05PM +0200, Martin Kletzander
wrote:
> This is the most trivial way to fix the issue with macOS not having
SOCK_CLOEXEC
> and SOCK_NONBLOCK.  There is not much better way, so this is the only way
to
> make it work on such platform(s).
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
> +int nbd_internal_socket(int domain,
> +                        int type,
> +                        int protocol,
> +                        bool nonblock)
> +{
> +  int fd;
> +
> +#ifdef __APPLE__
Feels too specific; better might be #ifndef SOCK_CLOEXEC.
> +  int flags;
> +#else
> +  type |= SOCK_CLOEXEC;
> +  if (nonblock)
> +    type |= SOCK_NONBLOCK;
> +#endif
> +
> +  fd = socket (domain, type, protocol);
> +
> +#ifdef __APPLE__
and again
> +  if (fd == -1)
> +    return -1;
> +
> +  if (fcntl (fd, F_SETFD, FD_CLOEXEC) == -1) {
> +    close(fd);
> +    return -1;
I'd add a comment documenting that this is a known race when libnbd is
used in a multithreaded program, but that we can't work around it
without Apple fixing their bug.
> +  }
> +
> +  if (nonblock) {
> +    flags = fcntl (fd, F_GETFL, 0);
> +    if (flags == -1 ||
> +        fcntl (fd, F_SETFL, flags|O_NONBLOCK) == -1) {
> +      close(fd);
> +      return -1;
> +    }
This one is not a race, merely an inconvenience.
> +  }
> +#endif
> +
> +  return fd;
> +}
> +
> +int
> +nbd_internal_socketpair (int domain, int type, int protocol, int *fds)
> +{
> +  int ret;
> +
> +#ifdef __APPLE__
and again
> +  size_t i;
> +#else
> +  type |= SOCK_CLOEXEC;
> +#endif
> +
> +  ret = socketpair (domain, type, protocol, fds);
> +
> +#ifdef __APPLE__
> +  if (ret == 0) {
> +    for (i = 0; i < 2; i++) {
> +      if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
> +        close(fds[0]);
> +        close(fds[1]);
> +        return -1;
> +      }
> +    }
> +  }
Do we care about the value of errno left in place after this function
fails?  Right now, we aren't very careful about what gets left there,
but if none of the callers care, we're okay.
> +#endif
> +
> +  return ret;
> +}
> diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c
> index 99a6d803258f..3d127e673e9e 100644
> --- a/fuzzing/libnbd-fuzz-wrapper.c
> +++ b/fuzzing/libnbd-fuzz-wrapper.c
> @@ -41,6 +41,34 @@
>  static void client (int s);
>  static void server (int fd, int s);
>  
> +static int
> +get_socketpair (int domain, int type, int protocol, int *fds)
> +{
> +  int ret;
> +
> +#ifdef __APPLE__
again for the #ifdef witness
> +  size_t i;
> +#else
> +  type |= SOCK_CLOEXEC;
> +#endif
> +
> +  ret = socketpair (domain, type, protocol, fds);
> +
> +#ifdef __APPLE__
> +  if (ret == 0) {
> +    for (i = 0; i < 2; i++) {
> +      if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
> +        close(fds[0]);
> +        close(fds[1]);
> +        return -1;
> +      }
> +    }
> +  }
Is the fuzzer multithreaded?  Or can we just blindly do the workaround
ourselves and not worry about trying to use SOCK_CLOEXEC here?  Also, it feels
like some duplication; can we not reuse nbd_internal_socketpair from the fuzzer?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Tue, Jul 13, 2021 at 11:26:05PM +0200, Martin Kletzander
wrote:
> This is the most trivial way to fix the issue with macOS not having
SOCK_CLOEXEC
> and SOCK_NONBLOCK.  There is not much better way, so this is the only way
to
> make it work on such platform(s).
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  lib/internal.h                               |  3 +
>  generator/states-connect-socket-activation.c |  2 +-
>  generator/states-connect.c                   | 11 ++--
>  lib/utils.c                                  | 68 ++++++++++++++++++++
>  fuzzing/libnbd-fuzz-wrapper.c                | 30 ++++++++-
>  fuzzing/libnbd-libfuzzer-test.c              | 30 ++++++++-
>  6 files changed, 136 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index 01f9d8ab5fea..8a4c189abe65 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -467,4 +467,7 @@ extern char *nbd_internal_printable_buffer (const void
*buf, size_t count);
>  extern char *nbd_internal_printable_string (const char *str);
>  extern char *nbd_internal_printable_string_list (char **list);
>  
> +extern int nbd_internal_socket(int domain, int type, int protocol, bool
nonblock);
> +extern int nbd_internal_socketpair(int domain, int type, int protocol, int
*fds);
Missing whitespace.

Also it would be useful to add a comment about what these functions do.
>  #endif /* LIBNBD_INTERNAL_H */
> diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
> index e601c9bb56be..8a2add312bc4 100644
> --- a/generator/states-connect-socket-activation.c
> +++ b/generator/states-connect-socket-activation.c
> @@ -131,7 +131,7 @@ STATE_MACHINE {
>      return 0;
>    }
>  
> -  s = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0);
> +  s = nbd_internal_socket (AF_UNIX, SOCK_STREAM, 0, false);
>    if (s == -1) {
>      SET_NEXT_STATE (%.DEAD);
>      set_error (errno, "socket");
> diff --git a/generator/states-connect.c b/generator/states-connect.c
> index fcac86f36a34..8de12183d627 100644
> --- a/generator/states-connect.c
> +++ b/generator/states-connect.c
> @@ -52,7 +52,7 @@ STATE_MACHINE {
>  
>    assert (!h->sock);
>    family = h->connaddr.ss_family;
> -  fd = socket (family, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0);
> +  fd = nbd_internal_socket (family, SOCK_STREAM, 0, true);
>    if (fd == -1) {
>      SET_NEXT_STATE (%.DEAD);
>      set_error (errno, "socket");
> @@ -162,9 +162,10 @@ STATE_MACHINE {
>      return -1;
>    }
>  
> -  fd = socket (h->rp->ai_family,
> -               h->rp->ai_socktype|SOCK_NONBLOCK|SOCK_CLOEXEC,
> -               h->rp->ai_protocol);
> +  fd = nbd_internal_socket (h->rp->ai_family,
> +                            h->rp->ai_socktype,
> +                            h->rp->ai_protocol,
> +                            true);
>    if (fd == -1) {
>      SET_NEXT_STATE (%NEXT_ADDRESS);
>      return 0;
> @@ -227,7 +228,7 @@ STATE_MACHINE {
>    assert (!h->sock);
>    assert (h->argv.ptr);
>    assert (h->argv.ptr[0]);
> -  if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) {
> +  if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
>      SET_NEXT_STATE (%.DEAD);
>      set_error (errno, "socketpair");
>      return 0;
> diff --git a/lib/utils.c b/lib/utils.c
> index 260fd6a25796..972cbc5208a7 100644
> --- a/lib/utils.c
> +++ b/lib/utils.c
> @@ -24,6 +24,7 @@
>  #include <unistd.h>
>  #include <ctype.h>
>  #include <errno.h>
> +#include <fcntl.h>
>  
>  #include "minmax.h"
>  
> @@ -258,3 +259,70 @@ nbd_internal_printable_string_list (char **list)
>    return s;
>  
>  }
> +
> +int nbd_internal_socket(int domain,
> +                        int type,
> +                        int protocol,
> +                        bool nonblock)
> +{
> +  int fd;
> +
> +#ifdef __APPLE__
> +  int flags;
> +#else
> +  type |= SOCK_CLOEXEC;
> +  if (nonblock)
> +    type |= SOCK_NONBLOCK;
> +#endif
As Eric mentioned already, it would be better to depend on whether
SOCK_CLOEXEC is defined rather than __APPLE__.

Are there platforms where SOCK_NONBLOCK is not defined?  I'm guessing
from the code below that MacOS is one such platform.
> +  fd = socket (domain, type, protocol);
> +
> +#ifdef __APPLE__
> +  if (fd == -1)
> +    return -1;
> +
> +  if (fcntl (fd, F_SETFD, FD_CLOEXEC) == -1) {
> +    close(fd);
> +    return -1;
> +  }
> +
> +  if (nonblock) {
> +    flags = fcntl (fd, F_GETFL, 0);
> +    if (flags == -1 ||
> +        fcntl (fd, F_SETFL, flags|O_NONBLOCK) == -1) {
> +      close(fd);
> +      return -1;
> +    }
> +  }
> +#endif
> +
> +  return fd;
> +}
> +
> +int
> +nbd_internal_socketpair (int domain, int type, int protocol, int *fds)
> +{
> +  int ret;
> +
> +#ifdef __APPLE__
> +  size_t i;
> +#else
> +  type |= SOCK_CLOEXEC;
> +#endif
> +
> +  ret = socketpair (domain, type, protocol, fds);
> +
> +#ifdef __APPLE__
> +  if (ret == 0) {
> +    for (i = 0; i < 2; i++) {
> +      if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
> +        close(fds[0]);
> +        close(fds[1]);
> +        return -1;
> +      }
> +    }
> +  }
> +#endif
> +
> +  return ret;
> +}
> diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c
> index 99a6d803258f..3d127e673e9e 100644
> --- a/fuzzing/libnbd-fuzz-wrapper.c
> +++ b/fuzzing/libnbd-fuzz-wrapper.c
We talked about this file in another part of the thread.
In this file (only) I believe it's better to just use:

  /* This test will never call exec(2). */
  #ifndef SOCK_CLOEXEC
  #define SOCK_CLOEXEC 0
  #endif
> diff --git a/fuzzing/libnbd-libfuzzer-test.c
b/fuzzing/libnbd-libfuzzer-test.c
> index 5ee29b877bdb..0bf988ee8398 100644
> --- a/fuzzing/libnbd-libfuzzer-test.c
> +++ b/fuzzing/libnbd-libfuzzer-test.c
I'm not sure about this one.  It could be that clang/libfuzzer uses
multiple threads.  But approximately no one cares about libfuzzer
(when compared to AFL++/Honggfuzz).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW