From 93d577eb6f87962de50ae9fe91ae110c3539171a Mon Sep 17 00:00:00 2001 From: Simon McVittie 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 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 +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