Re: Fix race in background worker termination for database - Mailing list pgsql-hackers

Hi,

While reading code for another work, I spotted something suspicious that might be an oversight in “[f1e251be8] Allow bgworkers to be terminated for database-related commands”.

In TerminateBackgroundWorkersForDatabase(Oid databaseId)
```
        /*
         * Iterate through slots, looking for workers connected to the given
         * database.
         */
        for (int slotno = 0; slotno < BackgroundWorkerData->total_slots; slotno++)
        {
                BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[slotno];

                if (slot->in_use &&
                        (slot->worker.bgw_flags & BGWORKER_INTERRUPTIBLE))
                {
                        PGPROC     *proc = BackendPidGetProc(slot->pid);

                        if (proc && proc->databaseId == databaseId)
                        {
                                slot->terminate = true;
                                signal_postmaster = true;

                                elog(DEBUG1, "termination requested for worker (PID %d) on database %u",
                                         (int) slot->pid, databaseId);
                        }
                }
        }
```

BackendPidGetProc() returns a PGPROC pointer after releasing ProcArrayLock, so the returned PGPROC is no longer protected. The corresponding entry in allProcs could be recycled after the function returns, in which case proc could refer to a different process when proc->databaseId is accessed.

Similarly, the postmaster may update slot->pid between the lookup and the elog(), so the logged PID has a small race as well.

I don’t have a concrete repro for this, but I think we should hold ProcArrayLock and use BackendPidGetProcWithLock() instead. See the attached patch for the changes.

One thing to point out is that, to keep the lock section as small as possible, I added a local variable, terminate, so that elog() can be placed outside the lock section. Maybe I’m overly cautious here, please let me know if that is unnecessary.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

The fix looks correct to me.  BackendPidGetProc() releases ProcArrayLock
before returning, so the returned PGPROC may be removed from the proc array
and reused before its databaseId is examined.  Keeping ProcArrayLock held
across both BackendPidGetProcWithLock() and the databaseId check closes
that race.

I suggest a small changes: