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
This commit is contained in:
Simon McVittie 2017-11-06 19:27:48 +00:00
parent 37d5af203c
commit 010223b6d2
3 changed files with 67 additions and 49 deletions

View file

@ -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

View file

@ -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,

View file

@ -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)
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_2;
}
if (!_dbus_noncefile_create (noncefile, error))
goto failed_3;
if (!_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;
}
}
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);