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/
v1-0001-Protect-PGPROC-lookup-when-terminating-background.patch
Description: Binary data
