g_dbus_connection_get_stream etc. are inherently dangerous
Submitted by Simon McVittie
Assigned to David Zeuthen
Link to original bug (#661679)
Description
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.