GNOME Bugzilla – Bug 661679
g_dbus_connection_get_stream etc. are inherently dangerous
Last modified: 2018-05-24 13:27:42 UTC
g_dbus_connection_get_stream() returns a stream that's in active use by the D-Bus worker thread; so does the equivalent property. The creator of the GDBusConnection could also keep a ref to a pre-existing stream and continue to use it from the main thread. I believe most (all?) GIOStream implementations are not thread-safe, following the general GObject policy that each object that isn't explicitly thread-safe may be used by at most one thread at a time. For instance, g_output_stream_set_pending doesn't access priv->closed or priv->pending atomically, and I doubt that real-world write_fn() implementations are thread-safe either. Even if the GIOStream is thread-safe, GDBusWorker assumes that it is the only user of its GIOStream: in particular, it asserts that !g_output_stream_has_pending(ostream) based on the knowledge that *it* doesn't have a write pending. (See Bug #651268.) I've used g_dbus_connection_get_stream() myself, in a dbus regression test, in order to use GDBus to implement the addressing logic and SASL handshake, then steal its underlying stream and send corrupt data. This is very useful for testing, but rather niche. Are there other valid uses for the underlying stream? To hijack the stream safely, you'd need a sequence something like this. I'm not particularly happy with the name "hijack", and particularly unhappy with "unhijack", but it's the best I could come up with... /** * g_dbus_connection_hijack_stream_sync: * @conn: a #GDBusConnection * @cancellable: a #GCancellable or %NULL * @error: return location for error or %NULL * * Flush the connection as if via g_dbus_connection_flush_sync(), * temporarily stop sending or receiving messages, and return the * underlying I/O stream. * * On success, the output substream is guaranteed to be * positioned at a D-Bus message boundary, and not have operations pending. * Until g_dbus_connection_unhijack_stream() is called, no messages will * be sent. g_dbus_connection_send_message() will queue messages * to be sent when the stream is unhijacked. * * Similarly, on success the input substream is guaranteed to be positioned * at a D-Bus message boundary, and not have operations pending. Any messages * that were read before this function returns will be dispatched from the * main loop as usual. After that, no more messages will be received until * g_dbus_connection_unhijack_stream() is called. * * If the connection is closed, raises %G_IO_ERROR_CLOSED. * * If the connection has not been initialized, raises * %G_IO_ERROR_NOT_INITIALIZED. * * If the connection's stream has already been hijacked, raises * %G_IO_ERROR_PENDING. * * Returns: the underlying #GIOStream */ /** * g_dbus_connection_unhijack_stream: * @conn: a connection * @stream: the I/O stream that was returned by * g_dbus_connection_hijack_stream_sync * * Resume processing of a stream previously hijacked by * g_dbus_connection_hijack_stream_sync(). * * When this method is called, the input and output substreams are both * assumed to be positioned at a message boundary. * * @stream must be exactly the same object that was previously returned: * this parameter only exists as a sanity-check. */ The semantics I suggest there for the input stream are a bit awkward, but that's only because I phrased the hijack function as synchronous; an async hijack function would dispatch all of the incoming messages before completing, One possible alternative: if the only use-cases involve injecting data into the output stream, it might be sufficient to just have a "hijack" API call, make no guarantees about the input stream at all, require the caller to flush beforehand if they don't want to lose messages, and have the GDBusConnection and GDBusWorker become useless (behave exactly as if they had been closed, perhaps?) after their stream is hijacked.
Another possible solution is: don't return the real underlying stream from g_dbus_connection_get_stream() or the property. Instead, return a wrapper (constructed on-demand) which performs all I/O by passing raw byte-blobs to and from the worker thread (it could even guarantee to act at a message boundary).
Created attachment 198951 [details] [review] GDBusConnection: warn that direct access to the stream is a bad idea Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661679 Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> --- This doesn't fix this bug completely, but documenting it is a step in the right direction.
Review of attachment 198951 [details] [review]: Looks good to me, please commit. Thanks!
(In reply to comment #0) > g_dbus_connection_get_stream() returns a stream that's in active use by the > D-Bus worker thread; so does the equivalent property. The creator of the > GDBusConnection could also keep a ref to a pre-existing stream and continue to > use it from the main thread. > > I believe most (all?) GIOStream implementations are not thread-safe, following > the general GObject policy that each object that isn't explicitly thread-safe > may be used by at most one thread at a time. For instance, > g_output_stream_set_pending doesn't access priv->closed or priv->pending > atomically, and I doubt that real-world write_fn() implementations are > thread-safe either. > > Even if the GIOStream is thread-safe, GDBusWorker assumes that it is the only > user of its GIOStream: in particular, it asserts that > !g_output_stream_has_pending(ostream) based on the knowledge that *it* doesn't > have a write pending. (See Bug #651268.) > > I've used g_dbus_connection_get_stream() myself, in a dbus regression test, in > order to use GDBus to implement the addressing logic and SASL handshake, then > steal its underlying stream and send corrupt data. This is very useful for > testing, but rather niche. Are there other valid uses for the underlying > stream? Not that I can think of. We probably should have made the :stream property non-readable and skipped its C getter g_dbus_connection_get_stream(). IIRC I added because it I thought it would be useful to get the socket address that way or something to do with symmetry - I forget. > To hijack the stream safely, you'd need a sequence something like this. I'm not > particularly happy with the name "hijack", and particularly unhappy with > "unhijack", but it's the best I could come up with... > > /** > * g_dbus_connection_hijack_stream_sync: > * @conn: a #GDBusConnection > * @cancellable: a #GCancellable or %NULL > * @error: return location for error or %NULL > * > * Flush the connection as if via g_dbus_connection_flush_sync(), > * temporarily stop sending or receiving messages, and return the > * underlying I/O stream. > * > * On success, the output substream is guaranteed to be > * positioned at a D-Bus message boundary, and not have operations pending. > * Until g_dbus_connection_unhijack_stream() is called, no messages will > * be sent. g_dbus_connection_send_message() will queue messages > * to be sent when the stream is unhijacked. > * > * Similarly, on success the input substream is guaranteed to be positioned > * at a D-Bus message boundary, and not have operations pending. Any messages > * that were read before this function returns will be dispatched from the > * main loop as usual. After that, no more messages will be received until > * g_dbus_connection_unhijack_stream() is called. > * > * If the connection is closed, raises %G_IO_ERROR_CLOSED. > * > * If the connection has not been initialized, raises > * %G_IO_ERROR_NOT_INITIALIZED. > * > * If the connection's stream has already been hijacked, raises > * %G_IO_ERROR_PENDING. > * > * Returns: the underlying #GIOStream > */ > > /** > * g_dbus_connection_unhijack_stream: > * @conn: a connection > * @stream: the I/O stream that was returned by > * g_dbus_connection_hijack_stream_sync > * > * Resume processing of a stream previously hijacked by > * g_dbus_connection_hijack_stream_sync(). > * > * When this method is called, the input and output substreams are both > * assumed to be positioned at a message boundary. > * > * @stream must be exactly the same object that was previously returned: > * this parameter only exists as a sanity-check. > */ > > The semantics I suggest there for the input stream are a bit awkward, but > that's only because I phrased the hijack function as synchronous; an async > hijack function would dispatch all of the incoming messages before completing, > > One possible alternative: if the only use-cases involve injecting data into the > output stream, it might be sufficient to just have a "hijack" API call, make no > guarantees about the input stream at all, require the caller to flush > beforehand if they don't want to lose messages, and have the GDBusConnection > and GDBusWorker become useless (behave exactly as if they had been closed, > perhaps?) after their stream is hijacked. Are you suggesting we should add this as public API? I'm not sure how that's useful except of course for testing... An easier alternative, perhaps, is to construct the GDBusConnection instance from a GIOStream-based instance comprised of GFilter{Input,Output}Stream acting on the streams from the original GIOStream (see gio/tests/gdbus-non-socket.c for an example). You'd need to check the message boundaries yourself here, though, but that isn't too hard given that you just read the first 16 bytes and then you know the length... We also don't currently support FD passing on anything but a GSocketConnection so this won't work with your filtered stream (if needed we can make this work though).
Comment on attachment 198951 [details] [review] GDBusConnection: warn that direct access to the stream is a bad idea mclasen applied this to master for 2.31. I applied this to 2.30 for 2.30.2 as well, since it's pure documentation. (Please revert if there's some reason not to have this.)
(In reply to comment #4) > An easier alternative, perhaps, is to construct the GDBusConnection instance > from a GIOStream-based instance comprised of GFilter{Input,Output}Stream acting > on the streams from the original GIOStream (see gio/tests/gdbus-non-socket.c > for an example). Sure, that'd be good enough for my regression test (if I can add a thread-safe "hijack" API onto my GFilterThingStream instance - otherwise it just moves the problem around). Or, I could restrict that test to only run on Unix, and dup() the underlying fd (I'm trying to corrupt the stream, so I don't mind if I accidentally interleave messages). (In reply to comment #4) > Not that I can think of. We probably should have made the :stream property > non-readable and skipped its C getter g_dbus_connection_get_stream(). Is there a standard place to make a note of this so we can do it in GLib 4? > IIRC I > added because it I thought it would be useful to get the socket address that > way or something to do with symmetry - I forget. Doing this in a thread-safe way would require extra guarantees on the GIOStream beyond the normal "you may not have more than one thread touching each GObject at a time" restriction, I think.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/465.