Re: Possible null pointer dereference in afterTriggerAddEvent()

On 2024-Jul-25, Alexander Kuznetsov wrote:

Hello Alexander,

> In src/backend/commands/trigger.c:4031, there is an
> afterTriggerAddEvent() function. The variable chunk is assigned the
> value of events->tail at line 4050. Subsequently, chunk is compared to
> NULL at lines 4051 and 4079, indicating that events->tail could
> potentially be NULL.
> 
> However, at line 4102, we dereference events->tail by accessing
> events->tail->next without first checking if it is NULL.

Thanks for reporting this.  I think the unwritten assumption is that
->tail and ->head are NULL or not simultaneously, so testing for one of
them is equivalent to testing both.  Failing to comply with this would
be data structure corruption.

> To address this issue, I propose at least adding an assertion to
> ensure that events->tail != NULL before the dereference. The suggested
> patch is included in the attachment.

Hmm, this doesn't actually change any behavior AFAICS.  If events->tail
is NULL and we reach that code, then the dereference to get ->next is
going to crash, whether the assertion is there or not.

Maybe for sanity (and perhaps for Svace compliance) we could do it the
other way around, i.e. by testing events->tail for nullness instead of
events->head, then add the assertion:

        if (events->tail == NULL)
        {
            Assert(events->head == NULL);
            events->head = chunk;
        }
        else
            events->tail->next = chunk;

This way, it's not wholly redundant.

That said, I'm not sure we actually *need* to change this.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)