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.

Agreed - fixed.

>
> + 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.

This call is consistent with the re-exec behaviour, but you are correct in that it's not technically required (and a facility to query state shouldn't potentially be changing it), so I've removed the call for this scenario.

>
> + 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.

This could indeed be written in a more efficient manner, but it's using the standard NIH idiom and is simple and clear I think. Also recall that this code only gets exercised in what is hopefully an impossible failure scenario anyway :-)

>
> 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).
>

Good catch! Had my socket cap on. Fixed :)

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

Thankfully not ;-)