thr3ads.net - Libguestfs - [Libguestfs] [libnbd PATCH v2 2/2] macOS: Simple cloexec/nonblock fix [Jul 2021]

If this information is useful, please help other people find it:
Share via:

Martin Kletzander

2021-Jul-27 11:31 UTC

[Libguestfs] [libnbd PATCH v2 2/2] macOS: Simple cloexec/nonblock fix

This is the most trivial way to fix the issue with macOS not having SOCK_CLOEXEC
and SOCK_NONBLOCK.  This is the only way to make it work on such platform(s)
unless they are fixed.

Signed-off-by: Martin Kletzander <mkletzan at redhat.com>

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

diff --git a/lib/internal.h b/lib/internal.h
index 01f9d8ab5fea..12938aaa0444 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -467,4 +467,11 @@ 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);
 
+/*
+ * These are wrappers over socket(2) and socketpair(2) that work on macOS where
+ * SOCK_NONBLOCK and SOCK_CLOEXEC are not available.
+ */
+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..5902c6ab016c 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,81 @@ nbd_internal_printable_string_list (char **list)
   return s;
 
 }
+
+int nbd_internal_socket(int domain,
+                        int type,
+                        int protocol,
+                        bool nonblock)
+{
+  int fd;
+
+  /* So far we do not know about any platform that has SOCK_CLOEXEC and lacks
+   * SOCK_NONBLOCK at the same time.
+   *
+   * The workaround for missing SOCK_CLOEXEC introduces a race which cannot be
+   * fixed until support for SOCK_CLOEXEC is added (or other fix is
implemented).
+   */
+#ifndef SOCK_CLOEXEC
+  int flags;
+#else
+  type |= SOCK_CLOEXEC;
+  if (nonblock)
+    type |= SOCK_NONBLOCK;
+#endif
+
+  fd = socket (domain, type, protocol);
+
+#ifndef SOCK_CLOEXEC
+  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;
+
+  /*
+   * Same as with nbd_internal_socket() this workaround for missing
SOCK_CLOEXEC
+   * introduces a race which cannot be fixed until support for SOCK_CLOEXEC is
+   * added (or other fix is implemented).
+   */
+#ifndef SOCK_CLOEXEC
+  size_t i;
+#else
+  type |= SOCK_CLOEXEC;
+#endif
+
+  ret = socketpair (domain, type, protocol, fds);
+
+#ifndef SOCK_CLOEXEC
+  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..eb188094cca7 100644
--- a/fuzzing/libnbd-fuzz-wrapper.c
+++ b/fuzzing/libnbd-fuzz-wrapper.c
@@ -38,6 +38,10 @@
 
 #include <libnbd.h>
 
+#ifndef SOCK_CLOEXEC
+#define SOCK_CLOEXEC 0 /* This file doesn't use exec */
+#endif
+
 static void client (int s);
 static void server (int fd, int s);
 
diff --git a/fuzzing/libnbd-libfuzzer-test.c b/fuzzing/libnbd-libfuzzer-test.c
index 5ee29b877bdb..c8d6423bf149 100644
--- a/fuzzing/libnbd-libfuzzer-test.c
+++ b/fuzzing/libnbd-libfuzzer-test.c
@@ -38,6 +38,10 @@
 
 #include <libnbd.h>
 
+#ifndef SOCK_CLOEXEC
+#define SOCK_CLOEXEC 0 /* This file doesn't use exec */
+#endif
+
 static void client (int sock);
 static void server (const uint8_t *data, size_t size, int sock);
 
-- 
2.32.0

Richard W.M. Jones

2021-Jul-27 12:52 UTC

head link

[Libguestfs] [libnbd PATCH v2 2/2] macOS: Simple cloexec/nonblock fix

On Tue, Jul 27, 2021 at 01:31:05PM +0200, Martin Kletzander
wrote:> This is the most trivial way to fix the issue with macOS not having
SOCK_CLOEXEC
> and SOCK_NONBLOCK.  This is the only way to make it work on such
platform(s)
> unless they are fixed.
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  lib/internal.h                               |  7 ++
>  generator/states-connect-socket-activation.c |  2 +-
>  generator/states-connect.c                   | 11 +--
>  lib/utils.c                                  | 79 ++++++++++++++++++++
>  fuzzing/libnbd-fuzz-wrapper.c                |  4 +
>  fuzzing/libnbd-libfuzzer-test.c              |  4 +
>  6 files changed, 101 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index 01f9d8ab5fea..12938aaa0444 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -467,4 +467,11 @@ 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);
>  
> +/*
> + * These are wrappers over socket(2) and socketpair(2) that work on macOS
where
> + * SOCK_NONBLOCK and SOCK_CLOEXEC are not available.
It's still a bit unclear.  Could we say:

/* These are wrappers around socket(2) and socketpair(2).  They
 * always set SOCK_CLOEXEC.  nbd_internal_socket can set SOCK_NONBLOCK
 * according to the nonblock parameter.
 */

Also, can you wrap lines at ~72 chars.
> +int nbd_internal_socket(int domain,
> +                        int type,
> +                        int protocol,
> +                        bool nonblock)
> +{
> +  int fd;
> +
> +  /* So far we do not know about any platform that has SOCK_CLOEXEC and
lacks
> +   * SOCK_NONBLOCK at the same time.
> +   *
> +   * The workaround for missing SOCK_CLOEXEC introduces a race which
cannot be
> +   * fixed until support for SOCK_CLOEXEC is added (or other fix is
implemented).
> +   */
Line wrapping here.
> +int
> +nbd_internal_socketpair (int domain, int type, int protocol, int *fds)
> +{
> +  int ret;
> +
> +  /*
> +   * Same as with nbd_internal_socket() this workaround for missing
SOCK_CLOEXEC
> +   * introduces a race which cannot be fixed until support for
SOCK_CLOEXEC is
> +   * added (or other fix is implemented).
> +   */
And here.

It's all good otherwise: ACK the series with the changes above.

Thanks,

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

Libguestfs - Jul 2021 - [libnbd PATCH v2 2/2] macOS: Simple cloexec/nonblock fix

[Libguestfs] [libnbd PATCH v2 2/2] macOS: Simple cloexec/nonblock fix

[Libguestfs] [libnbd PATCH v2 2/2] macOS: Simple cloexec/nonblock fix