Code review comment for lp:~jamesodhunt/upstart/bug-1079715

+ /* Get the relevant session */
+ session = session_from_dbus (NULL, message);
+
+ /* We don't want chroot sessions snooping outside their domain.
+ *
+ * Ideally, we'd allow them to query their own session, but the
+ * current implementation doesn't lend itself to that.
+ */
+ if (session && session->chroot) {
+ nih_warn (_("Ignoring state query from chroot session"));
+ return 0;
+ }
+
+ /* Disallow users from obtaining state details, unless they
+ * happen to own this process (which they may do in the test
+ * scenario and when running Upstart as a non-privileged user).
+ */
+ if (session && session->user && session->user != uid) {

Is there ever any possibility of a non-null session, where session->chroot is null and session->user is 0? If such a session exists, all users will be able to read it.

It's probably better to just do:

+ if (session && session->user != uid) {

this way, we always block access unless the uid matches - much more future-proof.

+ control_bus_flush ();

I'm not sure why this is needed in the GetState case. Is this because we can't serialize dbus state? Why would that be important in this context? Since we don't need to be able to deserialize the resulting state, I think this should be omitted unless not flushing somehow causes state_to_string() to fail.

+ while (TRUE) {
+ bytes = write (fd, buffer->buf, buffer->len);
+
+ if (! bytes)
+ break;
+ else if (bytes > 0)
+ nih_io_buffer_shrink (buffer, (size_t)bytes);
+ else if (bytes < 0 && (errno != EAGAIN && errno != EWOULDBLOCK)) {
+ break;
+ }
+ }
+

nih_io_buffer_shrink() seems an awfully inefficient way to do this, but I guess it's consistent with the rest of the code.

Shouldn't EINTR be handled here? Also, EWOULDBLOCK is AIUI only a specified behavior in the case of a socket, which we know this is not. So I think you want (errno != EAGAIN && errno != EINTR).

Otherwise, this looks good. I hope you didn't have to construct that json test data by hand ("Christmas"? :).