Re: pg_signal_backend() asymmetry

On Thu, Jun 28, 2012 at 6:48 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Thu, Jun 28, 2012 at 01:36:49AM -0700, Daniel Farina wrote:
>> On Wed, Jun 27, 2012 at 5:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> > I have one nitpick related to the recent changes for
>> > pg_cancel_backend() and pg_terminate_backend(). If you use these
>> > functions as an unprivileged user, and try to signal a nonexistent
>> > PID, you get:
>>
>> I think the goal there is to avoid leakage of the knowledge or
>> non-knowledge of a given PID existing once it is deemed out of
>> Postgres' control.  Although I don't have a specific attack vector in
>> mind for when one knows a PID exists a-priori, it does seem like an
>> unnecessary admission on the behalf of other programs.
>
> I think it was just an oversight.  I agree that these functions have no
> business helping users probe for live non-PostgreSQL PIDs on the server, but
> they don't do so and Josh's patch won't change that.  I recommend committing
> the patch.  Users will be able to probe for live PostgreSQL PIDs, but
> pg_stat_activity already provides those.

Yeah, I figured that since pg_stat_activity already shows users PIDs,
there should be no need to have pg_signal_backend() lie about whether
a PID was a valid backend or not. And if for some reason we did want
to lie, we should give an accurate message like "not valid backend, or
must be superuser or have the same role...".

I noticed that I neglected to update the comment which was in the
non-superuser case: updated version attached.

On Thu, Jun 28, 2012 at 5:22 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Well, there are two sides to it - one is the message, the other is if
> it should be ERROR or WARNING. My reading is that Josh is suggesting
> we make them WARNING in both cases, right?

Maybe I should have said I had "a few" nitpicks instead of just "one"
:-) A more detailed summary of the little issues I'm aiming to fix:

1a.) SIGNAL_BACKEND_NOPERMISSION being used for both invalid PID and
role mismatch, causing the "must be superuser or have ..." message to
be printed in both cases for non-superusers

1b.) Since SIGNAL_BACKEND_NOPERMISSION is used for both duties, you'll
get an ERROR instead of just a WARNING during plausibly-normal usage.
For example, if you're running
SELECT pg_cancel_backend(pid)
FROM pg_stat_activity
WHERE usename = CURRENT_USER AND
pid != pg_backend_pid();

you might be peeved if it ERROR'ed out with the bogus message claiming
the process was owned by someone else, when the backend has just
exited on its own. So even if we thought it was worth hiding to
non-superusers whether the PID is invalid or belongs to someone else,
I think the message should just be at WARNING level.

2a.) Existing code uses two calls to BackendPidGetProc() for
non-superusers in the error-free case.

2b.) I think the comment "the check for whether it's a backend process
is below" is a little misleading, since IsBackendPid() is just
checking whether the return of BackendPidGetProc() is NULL, which has
already been done.

3.) grammar fix for comment ("on it's own" -> "on its own")

Josh