Merge branch 'wip/smcv/autofd' into 'master'

backports, local-alloc: Provide a backport of g_autofd

See merge request GNOME/libglnx!68

glnx-backports.h

@@ -95,6 +95,27 @@ _glnx_clear_fd (int     *fd_ptr,
 
 #endif
 
+/* This is part of the backport of g_autofd, but we define it
+ * unconditionally because it's also used to implement glnx_close_fd() */
+static inline void
+_glnx_clear_fd_ignore_error (int *fd_ptr)
+{
+  /* Don't overwrite thread-local errno if closing the fd fails */
+  int errsv = errno;
+
+  if (!g_clear_fd (fd_ptr, NULL))
+    {
+      /* Do nothing: we ignore all errors, except for EBADF which
+       * is a programming error, checked for by g_close(). */
+    }
+
+  errno = errsv;
+}
+
+#if !GLIB_CHECK_VERSION(2, 76, 0)
+#define g_autofd __attribute__((cleanup(_glnx_clear_fd_ignore_error)))
+#endif
+
 #if !GLIB_CHECK_VERSION(2, 40, 0)
 #define g_info(...) g_log (G_LOG_DOMAIN, G_LOG_LEVEL_INFO, __VA_ARGS__)
 #endif

glnx-local-alloc.h

@@ -51,38 +51,23 @@ glnx_local_obj_unref (void *v)
  * glnx_close_fd:
  * @fdp: Pointer to fd
  *
- * Effectively `close (g_steal_fd (&fd))`.  Also
- * asserts that `close()` did not raise `EBADF` - encountering
- * that error is usually a critical bug in the program.
+ * Same as `g_clear_fd()`, but ignoring the error (if any) and making sure
+ * not to alter `errno`. As a result, this function can be used for cleanup
+ * in contexts where `errno` needs to be preserved.
  */
-static inline void
-glnx_close_fd (int *fdp)
-{
-  int errsv;
-
-  g_assert (fdp);
-
-  int fd = g_steal_fd (fdp);
-  if (fd >= 0)
-    {
-      errsv = errno;
-      if (close (fd) < 0)
-        g_assert (errno != EBADF);
-      errno = errsv;
-    }
-}
+#define glnx_close_fd _glnx_clear_fd_ignore_error
 
 /**
  * glnx_fd_close:
  *
- * Deprecated in favor of `glnx_autofd`.
+ * Deprecated in favor of `g_autofd`.
  */
-#define glnx_fd_close __attribute__((cleanup(glnx_close_fd)))
+#define glnx_fd_close g_autofd
 /**
  * glnx_autofd:
  *
- * Call close() on a variable location when it goes out of scope.
+ * Deprecated in favor of `g_autofd`.
  */
-#define glnx_autofd __attribute__((cleanup(glnx_close_fd)))
+#define glnx_autofd g_autofd
 
 G_END_DECLS

tests/libglnx-testlib.c

@@ -23,6 +23,7 @@
 #include "libglnx-testlib.h"
 
 #include <errno.h>
+#include <fcntl.h>
 
 #include <glib/gstdio.h>
 
@@ -72,3 +73,21 @@ _glnx_test_auto_temp_dir_leave (_GLnxTestAutoTempDir *dir)
   g_free (dir->old_cwd);
   g_free (dir);
 }
+
+void
+_glnx_test_assert_fd_was_closed (int fd)
+{
+  /* We can't tell a fd was really closed without behaving as though it
+   * was still valid */
+  if (g_test_undefined ())
+    {
+      int result;
+      int errsv;
+
+      errno = 0;
+      result = fcntl (fd, F_GETFD, 0);
+      errsv = errno;
+      g_assert_cmpint (result, <, 0);
+      g_assert_cmpint (errsv, ==, EBADF);
+    }
+}

tests/libglnx-testlib.h

@@ -1,7 +1,7 @@
 /* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*-
  *
  * Copyright (C) 2017 Red Hat, Inc.
- * Copyright 2019 Collabora Ltd.
+ * Copyright 2019-2022 Collabora Ltd.
  * SPDX-License-Identifier: LGPL-2.0-or-later
  *
  * This library is free software; you can redistribute it and/or
@@ -23,6 +23,7 @@
 #pragma once
 
 #include <glib.h>
+#include <glib/gstdio.h>
 
 #include "glnx-backport-autoptr.h"
 
@@ -47,3 +48,5 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(_GLnxTestAutoTempDir, _glnx_test_auto_temp_dir_lea
 
 #define _GLNX_TEST_SCOPED_TEMP_DIR \
   G_GNUC_UNUSED g_autoptr(_GLnxTestAutoTempDir) temp_dir = _glnx_test_auto_temp_dir_enter ()
+
+void _glnx_test_assert_fd_was_closed (int fd);

tests/test-libglnx-fdio.c

@@ -29,6 +29,125 @@
 
 #include "libglnx-testlib.h"
 
+static void
+test_close (void)
+{
+  g_autoptr(GError) error = NULL;
+  int errsv;
+  int fd = -2;
+  int fd_borrowed;
+
+  g_test_summary ("Exercise glnx_close_fd");
+
+  /* Closing a non-fd is a no-op, and preserves errno.
+   * EILSEQ is an arbitrary valid value of errno that is unlikely
+   * to be set accidentally as a side-effect of I/O. */
+  g_test_message ("Closing a non-fd is a no-op and preserves errno...");
+  errno = EILSEQ;
+  glnx_close_fd (&fd);
+  errsv = errno;
+  g_assert_cmpint (fd, ==, -1);
+  g_assert_cmpint (errsv, ==, EILSEQ);
+
+  /* Closing a valid fd really closes it, and preserves errno. */
+  g_test_message ("Closing a valid fd preserves errno...");
+  glnx_opendirat (AT_FDCWD, "/", TRUE, &fd, &error);
+  g_assert_no_error (error);
+  g_assert_cmpint (fd, >=, 0);
+  fd_borrowed = fd;
+  errno = EILSEQ;
+  glnx_close_fd (&fd);
+  errsv = errno;
+  g_assert_cmpint (fd, ==, -1);
+  g_assert_cmpint (errsv, ==, EILSEQ);
+  _glnx_test_assert_fd_was_closed (fd_borrowed);
+}
+
+/* Exercise glnx_close_fd in the case where close() fails.
+ * The only convenient way we can arrange for this to happen is to use
+ * an invalid fd, which is a programming error.
+ *
+ * This function is only run under g_test_undefined(), and assumes the
+ * implementation detail that GLib responds to that programming error
+ * with a critical warning rather than a fatal error. */
+static void
+test_close_ebadf_subprocess (void)
+{
+  g_autoptr(GError) error = NULL;
+  int errsv;
+  int fd = -2;
+  int non_fd;
+
+  /* Preparation: Open a fd, and close it, leaving non_fd set to the
+   * file descriptor number. */
+  glnx_opendirat (AT_FDCWD, "/", TRUE, &fd, &error);
+  g_assert_no_error (error);
+  g_assert_cmpint (fd, >=, 0);
+  close (fd);
+  non_fd = fd;
+  _glnx_test_assert_fd_was_closed (non_fd);
+
+  /* "Closing" the non-fd provokes a critical warning. */
+  g_log_set_always_fatal (G_LOG_FATAL_MASK);
+  g_log_set_fatal_mask ("GLib", G_LOG_FATAL_MASK);
+
+  errno = EILSEQ;
+  glnx_close_fd (&non_fd);
+  errsv = errno;
+  g_assert_cmpint (non_fd, ==, -1);
+
+  /* We preserved errno. */
+  g_assert_cmpint (errsv, ==, EILSEQ);
+  g_print ("Closing invalid fd preserved errno\n");
+}
+
+static void
+test_close_ebadf (void)
+{
+  g_test_summary ("Exercise glnx_close_fd when close() fails");
+
+  /* If close() fails, it still preserves errno.
+   * The only convenient way to make close() fail on-demand is EBADF. */
+
+  if (g_test_subprocess ())
+    {
+      test_close_ebadf_subprocess ();
+      return;
+    }
+
+  if (g_test_undefined ())
+    {
+      g_test_message ("Closing invalid fd preserves errno...");
+
+#if GLIB_CHECK_VERSION (2, 38, 0)
+      g_test_trap_subprocess (NULL, 0, G_TEST_SUBPROCESS_DEFAULT);
+#else
+      if (g_test_trap_fork (0, 0))
+        {
+          test_close_ebadf_subprocess ();
+          _exit (0);
+        }
+#endif
+
+      g_test_trap_assert_passed ();
+      g_test_trap_assert_stdout ("*Closing invalid fd preserved errno*");
+#if !GLIB_CHECK_VERSION(2, 76, 0)
+      /* We can assert that our backport emits this message */
+      g_test_trap_assert_stderr ("*_glnx_close(fd:*) failed with EBADF*");
+#else
+      /* We can't assert anything this specific about GLib's,
+       * but as an implementation detail, it's currently very similar */
+# if 0
+      g_test_trap_assert_stderr ("*g_close(fd:*) failed with EBADF*");
+# endif
+#endif
+    }
+  else
+    {
+      g_test_skip ("Can't test this without provoking undefined behaviour");
+    }
+}
+
 static gboolean
 renameat_test_setup (int *out_srcfd, int *out_destfd,
                      GError **error)
@@ -445,6 +564,8 @@ int main (int argc, char **argv)
 
   g_test_init (&argc, &argv, NULL);
 
+  g_test_add_func ("/close", test_close);
+  g_test_add_func ("/close/ebadf", test_close_ebadf);
   g_test_add_func ("/tmpfile", test_tmpfile);
   g_test_add_func ("/stdio-file", test_stdio_file);
   g_test_add_func ("/filecopy", test_filecopy);