Attachment 135283 Details for Bug 103597 – DBusNonceFile: Don't rely on caller preallocating the object

[patch] DBusNonceFile: Don't rely on caller preallocating the object

0001-DBusNonceFile-Don-t-rely-on-caller-preallocating-the.patch (text/plain), 9.53 KB, created by Simon McVittie on 2017-11-07 12:20:24 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2017-11-07 12:20:24 UTC

Size: 9.53 KB

patch

obsolete

>From 93d577eb6f87962de50ae9fe91ae110c3539171a Mon Sep 17 00:00:00 2001
>From: Simon McVittie <smcv@collabora.com>
>Date: Mon, 6 Nov 2017 19:27:48 +0000
>Subject: [PATCH] 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>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=103597
>---
> dbus/dbus-nonce.c         | 68 ++++++++++++++++++++++++++++++++++++++---------
> dbus/dbus-nonce.h         | 10 ++-----
> dbus/dbus-server-socket.c | 38 +++++++-------------------
> 3 files changed, 67 insertions(+), 49 deletions(-)
>
>diff --git a/dbus/dbus-nonce.c b/dbus/dbus-nonce.c
>index eab23f64..146099f4 100644
>--- 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 99366fd4..bf32af80 100644
>--- 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 d716f500..04ab05f7 100644
>--- 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);
>-- 
>2.15.0
>

Actions: View | Splinter Review

Attachments on bug 103597: 135266 | 135267 | 135268 | 135269 | 135283

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.