Re: [PATCH] avoid buffer underflow in errfinish()

On 27.03.2013 14:50, Robert Haas wrote:
> On Sat, Mar 23, 2013 at 6:45 PM, Xi Wang<xi(dot)wang(at)gmail(dot)com> wrote:
>> A side question: at src/backend/storage/lmgr/proc.c:1150, is there a
>> null pointer deference for `autovac'?

I think you mean on line 1142:

> PGPROC *autovac = GetBlockingAutoVacuumPgproc();
> *HERE* PGXACT *autovac_pgxact = &ProcGlobal->allPgXact[autovac->pgprocno];
>
> LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
>
> /*
> * Only do it if the worker is not working to protect against Xid
> * wraparound.
> */
> if ((autovac != NULL) &&
> (autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&
> !(autovac_pgxact->vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))

> Not really. If the deadlock_state is DS_BLOCKED_BY_AUTOVACUUM, there
> has to be a blocking autovacuum proc. As in the other case that you
> found, though, some code rearrangement would likely make the intent of
> the code more clear and avoid future mistakes.
>
> Perhaps something like:
>
> if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM&&
> allow_autovacuum_cancel
> && (autovac = GetBlockingAutoVacuumPgproc()) != NULL)
> {
> PGXACT *autovac_pgxact =
> &ProcGlobal->allPgXact[autovac->pgprocno];
> ...

Writing it like that suggests that autovac might sometimes be NULL, even
if deadlock_state == DS_BLOCKED_BY_AUTOVACUUM. From your explanation
above, I gather that's not possible (and I think you're right), so the
NULL check is unnecessary. If we think it might be NULL after all, the
above makes sense.

- Heikki