On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith <greg@2ndquadrant.com> wrote:
> Same-user cancels, but not termination. Only this, and nothing more.
+1 from me on this approach. I think enough people have clamored for
this simple approach which solves the common-case.
> There's one obvious and questionable design decision I made to highlight.
> Right now the only consumers of pg_signal_backend are the cancel and
> terminate calls. What I did was make pg_signal_backend more permissive,
> adding the idea that role equivalence = allowed, and therefore granting that
> to anything else that might call it. And then I put a stricter check on
> termination. This results in a redundant check of superuser on the
> termination check, and the potential for mis-using pg_signal_backend. . .
I think that's OK, it should be easy enough to reorganize if more
callers to pg_signal_backend() happen to come along.
The only niggling concern I have about this patch (and, I think, all
similar ones proposed) is the possible race condition between the
permissions checking and the actual call of kill() inside
pg_signal_backend(). I fooled around with this by using gdb to attach
to Backend #1, stuck a breakpoint right before the call to:
if (kill(-pid, sig))
and ran a pg_cancel_backend() of a same-role Backend #2. Then, with
the permissions checking passed, I exited Backend #2 and used a script
to call fork() in a loop until my system PIDs had wrapped around to a
few PIDs before the one Backend #2 had held. Then, I opened a few
superuser connections until I had one with the same PID which Backend
#2 previously had.
After I released the breakpoint in gdb on Backend #1, it happily sent
a SIGINT to my superuser backend.
I notice that BackendPidGetProc() has the comment:
... Note that it is up to the caller to be sure that the question remains meaningful for long enough for the answer
tobe used ...
So someone has mulled this over before. It took my script maybe 15-20
minutes to cause a wraparound by running fork() in a loop, and that
wraparound would somehow have to occur within the short execution of
pg_signal_backend(). I'm not super worried about the patch from a
security perspective, just thought I'd point this out.
Josh
