DBusNonceFile: Don't rely on caller preallocating the object

If we combine the dbus_new0, populating the DBusString members and the
actual creation of the file, RAII-style, then we never need to worry
about a partially-initialized or uninitialized DBusNonceFile becoming
visible to a caller.

Similarly, if we combine deletion of the file, freeing of the
DBusString members, freeing the structure and clearing the pointer to
the structure, then we can never be in an inconsistent situation,
except during the actual implementation of _dbus_noncefile_delete().

Note that there are two implementations each of
_dbus_noncefile_create() and _dbus_noncefile_delete(). This is because
on Unix we must use a subdirectory of _dbus_get_tmpdir() (the nonce
filename is not created atomically, so that would not be safe), while
on Windows we use the directory directly (the Windows temp directory
is private to a user, so this is OK).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Reviewed-by: Philip Withnall <withnall@endlessm.com>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=103597

diff --git a/dbus/dbus-nonce.c b/dbus/dbus-nonce.c
index eab23f6424e1aaa15dc0a9deac47a67f9a733fcc..146099f4a3763dee8c71ba2210a07354dda23608 100644 (file)
--- a/dbus/dbus-nonce.c
+++ b/dbus/dbus-nonce.c
@@ -30,6 +30,12 @@
 
 #include <stdio.h>
 
+struct DBusNonceFile
+{
+  DBusString path;
+  DBusString dir;
+};
+
 static dbus_bool_t
 do_check_nonce (DBusSocket fd, const DBusString *nonce, DBusError *error)
 {
@@ -281,16 +287,20 @@ _dbus_send_nonce (DBusSocket        fd,
 }
 
 static dbus_bool_t
-do_noncefile_create (DBusNonceFile *noncefile,
+do_noncefile_create (DBusNonceFile **noncefile_out,
                      DBusError *error,
                      dbus_bool_t use_subdir)
 {
+    DBusNonceFile *noncefile = NULL;
     DBusString randomStr;
     const char *tmp;
 
     _DBUS_ASSERT_ERROR_IS_CLEAR (error);
 
-    _dbus_assert (noncefile);
+    _dbus_assert (noncefile_out != NULL);
+    _dbus_assert (*noncefile_out == NULL);
+
+    noncefile = dbus_new0 (DBusNonceFile, 1);
 
     /* Make it valid to "free" these even if _dbus_string_init() runs
      * out of memory: see comment in do_check_nonce() */
@@ -363,6 +373,7 @@ do_noncefile_create (DBusNonceFile *noncefile,
       }
     _DBUS_ASSERT_ERROR_IS_CLEAR (error);
 
+    *noncefile_out = noncefile;
     _dbus_string_free (&randomStr);
 
     return TRUE;
@@ -371,6 +382,7 @@ do_noncefile_create (DBusNonceFile *noncefile,
       _dbus_delete_directory (&noncefile->dir, NULL);
     _dbus_string_free (&noncefile->dir);
     _dbus_string_free (&noncefile->path);
+    dbus_free (noncefile);
     _dbus_string_free (&randomStr);
     return FALSE;
 }
@@ -379,33 +391,49 @@ do_noncefile_create (DBusNonceFile *noncefile,
 /**
  * creates a nonce file in a user-readable location and writes a generated nonce to it
  *
- * @param noncefile returns the nonce file location
+ * @param noncefile_out returns the nonce file location
  * @param error error details if creating the nonce file fails
  * @return TRUE iff the nonce file was successfully created
  */
 dbus_bool_t
-_dbus_noncefile_create (DBusNonceFile *noncefile,
+_dbus_noncefile_create (DBusNonceFile **noncefile_out,
                         DBusError *error)
 {
-    return do_noncefile_create (noncefile, error, /*use_subdir=*/FALSE);
+    return do_noncefile_create (noncefile_out, error, /*use_subdir=*/FALSE);
 }
 
 /**
  * deletes the noncefile and frees the DBusNonceFile object.
  *
- * @param noncefile the nonce file to delete. Contents will be freed.
+ * If noncefile_location points to #NULL, nothing is freed or deleted,
+ * similar to dbus_error_free().
+ *
+ * @param noncefile_location the nonce file to delete. Contents will be freed and cleared to #NULL.
  * @param error error details if the nonce file could not be deleted
  * @return TRUE
  */
 dbus_bool_t
-_dbus_noncefile_delete (DBusNonceFile *noncefile,
+_dbus_noncefile_delete (DBusNonceFile **noncefile_location,
                         DBusError *error)
 {
+    DBusNonceFile *noncefile;
+
     _DBUS_ASSERT_ERROR_IS_CLEAR (error);
+    _dbus_assert (noncefile_location != NULL);
+
+    noncefile = *noncefile_location;
+    *noncefile_location = NULL;
+
+    if (noncefile == NULL)
+      {
+        /* Nothing to do */
+        return TRUE;
+      }
 
     _dbus_delete_file (&noncefile->path, error);
     _dbus_string_free (&noncefile->dir);
     _dbus_string_free (&noncefile->path);
+    dbus_free (noncefile);
     return TRUE;
 }
 
@@ -414,33 +442,49 @@ _dbus_noncefile_delete (DBusNonceFile *noncefile,
  * creates a nonce file in a user-readable location and writes a generated nonce to it.
  * Initializes the noncefile object.
  *
- * @param noncefile returns the nonce file location
+ * @param noncefile_out returns the nonce file location
  * @param error error details if creating the nonce file fails
  * @return TRUE iff the nonce file was successfully created
  */
 dbus_bool_t
-_dbus_noncefile_create (DBusNonceFile *noncefile,
+_dbus_noncefile_create (DBusNonceFile **noncefile_out,
                         DBusError *error)
 {
-    return do_noncefile_create (noncefile, error, /*use_subdir=*/TRUE);
+    return do_noncefile_create (noncefile_out, error, /*use_subdir=*/TRUE);
 }
 
 /**
  * deletes the noncefile and frees the DBusNonceFile object.
  *
- * @param noncefile the nonce file to delete. Contents will be freed.
+ * If noncefile_location points to #NULL, nothing is freed or deleted,
+ * similar to dbus_error_free().
+ *
+ * @param noncefile_location the nonce file to delete. Contents will be freed and cleared to #NULL.
  * @param error error details if the nonce file could not be deleted
  * @return TRUE
  */
 dbus_bool_t
-_dbus_noncefile_delete (DBusNonceFile *noncefile,
+_dbus_noncefile_delete (DBusNonceFile **noncefile_location,
                         DBusError *error)
 {
+    DBusNonceFile *noncefile;
+
     _DBUS_ASSERT_ERROR_IS_CLEAR (error);
+    _dbus_assert (noncefile_location != NULL);
+
+    noncefile = *noncefile_location;
+    *noncefile_location = NULL;
+
+    if (noncefile == NULL)
+      {
+        /* Nothing to do */
+        return TRUE;
+      }
 
     _dbus_delete_directory (&noncefile->dir, error);
     _dbus_string_free (&noncefile->dir);
     _dbus_string_free (&noncefile->path);
+    dbus_free (noncefile);
     return TRUE;
 }
 #endif

diff --git a/dbus/dbus-nonce.h b/dbus/dbus-nonce.h
index 99366fd4aba705ac55183dcd25e3d3c67c1a6e02..bf32af80f5826c50f0dd09c16b15a2504892ee61 100644 (file)
--- a/dbus/dbus-nonce.h
+++ b/dbus/dbus-nonce.h
@@ -33,18 +33,12 @@ DBUS_BEGIN_DECLS
 
 typedef struct DBusNonceFile DBusNonceFile;
 
-struct DBusNonceFile
-{
-  DBusString path;
-  DBusString dir;
-};
-
 // server
 
-dbus_bool_t _dbus_noncefile_create (DBusNonceFile *noncefile,
+dbus_bool_t _dbus_noncefile_create (DBusNonceFile **noncefile_out,
                                     DBusError *error);
 
-dbus_bool_t _dbus_noncefile_delete (DBusNonceFile *noncefile,
+dbus_bool_t _dbus_noncefile_delete (DBusNonceFile **noncefile_location,
                                     DBusError *error);
 
 dbus_bool_t _dbus_noncefile_check_nonce (DBusSocket fd,

diff --git a/dbus/dbus-server-socket.c b/dbus/dbus-server-socket.c
index d716f500234d327456595cac4e7d350fd5edf568..04ab05f7363ac0a904f2ad73628f223cee202ebb 100644 (file)
--- a/dbus/dbus-server-socket.c
+++ b/dbus/dbus-server-socket.c
@@ -75,9 +75,7 @@ socket_finalize (DBusServer *server)
   dbus_free (socket_server->fds);
   dbus_free (socket_server->watch);
   dbus_free (socket_server->socket_name);
-  if (socket_server->noncefile)
-       _dbus_noncefile_delete (socket_server->noncefile, NULL);
-  dbus_free (socket_server->noncefile);
+  _dbus_noncefile_delete (&socket_server->noncefile, NULL);
   dbus_free (server);
 }
 
@@ -409,7 +407,7 @@ _dbus_server_new_for_tcp_socket (const char     *host,
   DBusString address;
   DBusString host_str;
   DBusString port_str;
-  DBusNonceFile *noncefile;
+  DBusNonceFile *noncefile = NULL;
   
   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
 
@@ -466,47 +464,29 @@ _dbus_server_new_for_tcp_socket (const char     *host,
 
   if (use_nonce)
     {
-      noncefile = dbus_new0 (DBusNonceFile, 1);
-      if (noncefile == NULL)
-        {
-          dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          goto failed_2;
-        }
-
-      if (!_dbus_noncefile_create (noncefile, error))
-          goto failed_3;
-
-      if (!_dbus_string_append (&address, ",noncefile=") ||
+      if (!_dbus_noncefile_create (&noncefile, error) ||
+          !_dbus_string_append (&address, ",noncefile=") ||
           !_dbus_address_append_escaped (&address, _dbus_noncefile_get_path (noncefile)))
         {
           dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          goto failed_4;
+          goto failed_2;
         }
-
     }
 
   server = _dbus_server_new_for_socket (listen_fds, nlisten_fds, &address, noncefile, error);
   if (server == NULL)
-    {
-      if (noncefile != NULL)
-        goto failed_4;
-      else
-        goto failed_2;
-    }
+    goto failed_2;
 
+  /* server has taken ownership of noncefile and the fds in listen_fds */
   _dbus_string_free (&port_str);
   _dbus_string_free (&address);
   dbus_free(listen_fds);
 
   return server;
 
- failed_4:
-  _dbus_noncefile_delete (noncefile, NULL);
-
- failed_3:
-  dbus_free (noncefile);
-
  failed_2:
+  _dbus_noncefile_delete (&noncefile, NULL);
+
   for (i = 0 ; i < nlisten_fds ; i++)
     _dbus_close_socket (listen_fds[i], NULL);
   dbus_free(listen_fds);