Have pg_terminate/cancel_backend not ERROR on non-existent processes

This worked fine for superusers, but not for ordinary users trying to
cancel their own processes.  Tweak the order the checks are done in so
that we correctly return SIGNAL_BACKEND_ERROR (which current callers
know to ignore without erroring out) so that an ordinary user can loop
through a resultset without fearing that a process might exit in the
middle of said looping -- causing the remaining processes to go
unsignalled.

Incidentally, the last in-core caller of IsBackendPid() is now gone.
However, the function is exported and must remain in place, because
there are plenty of callers in external modules.

Author: Josh Kupershmidt

Reviewed by Noah Misch

diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index a501bb3b74575942e3e985338b32430a7eee7156..cd20b838416834461a46ca5829a949c7bba50d6b 100644 (file)
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -73,12 +73,13 @@ current_query(PG_FUNCTION_ARGS)
 
 /*
  * Send a signal to another backend.
+ *
  * The signal is delivered if the user is either a superuser or the same
  * role as the backend being signaled. For "dangerous" signals, an explicit
  * check for superuser needs to be done prior to calling this function.
  *
  * Returns 0 on success, 1 on general failure, and 2 on permission error.
- * In the event of a general failure (returncode 1), a warning message will
+ * In the event of a general failure (return code 1), a warning message will
  * be emitted. For permission errors, doing that is the responsibility of
  * the caller.
  */
@@ -88,38 +89,30 @@ current_query(PG_FUNCTION_ARGS)
 static int
 pg_signal_backend(int pid, int sig)
 {
-   PGPROC     *proc;
-
-   if (!superuser())
-   {
-       /*
-        * Since the user is not superuser, check for matching roles. Trust
-        * that BackendPidGetProc will return NULL if the pid isn't valid,
-        * even though the check for whether it's a backend process is below.
-        * The IsBackendPid check can't be relied on as definitive even if it
-        * was first. The process might end between successive checks
-        * regardless of their order. There's no way to acquire a lock on an
-        * arbitrary process to prevent that. But since so far all the callers
-        * of this mechanism involve some request for ending the process
-        * anyway, that it might end on its own first is not a problem.
-        */
-       proc = BackendPidGetProc(pid);
+   PGPROC     *proc = BackendPidGetProc(pid);
 
-       if (proc == NULL || proc->roleId != GetUserId())
-           return SIGNAL_BACKEND_NOPERMISSION;
-   }
-
-   if (!IsBackendPid(pid))
+   /*
+    * BackendPidGetProc returns NULL if the pid isn't valid; but by the time
+    * we reach kill(), a process for which we get a valid proc here might have
+    * terminated on its own.  There's no way to acquire a lock on an arbitrary
+    * process to prevent that. But since so far all the callers of this
+    * mechanism involve some request for ending the process anyway, that it
+    * might end on its own first is not a problem.
+    */
+   if (proc == NULL)
    {
        /*
         * This is just a warning so a loop-through-resultset will not abort
-        * if one backend terminated on it's own during the run
+        * if one backend terminated on its own during the run.
         */
        ereport(WARNING,
                (errmsg("PID %d is not a PostgreSQL server process", pid)));
        return SIGNAL_BACKEND_ERROR;
    }
 
+   if (!(superuser() || proc->roleId == GetUserId()))
+       return SIGNAL_BACKEND_NOPERMISSION;
+
    /*
     * Can the process we just validated above end, followed by the pid being
     * recycled for a new process, before reaching here?  Then we'd be trying