Обсуждение: Possible null pointer dereference in afterTriggerAddEvent()

Hello everyone,

In src/backend/commands/trigger.c:4031, there is an afterTriggerAddEvent() function. The variable chunk is assigned the
valueof events->tail at line 4050. Subsequently, chunk is compared to NULL at lines 4051 and 4079, indicating that
events->tailcould potentially be NULL.
 

However, at line 4102, we dereference events->tail by accessing events->tail->next without first checking if it is
NULL.

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.
 

-- 
Best regards,
Alexander Kuznetsov

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)

25.07.2024 20:07, Alvaro Herrera wrote:
> 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.
Thanks for your response!
I agree with the proposed changes and have updated the patch accordingly. Version 2 is attached.
> That said, I'm not sure we actually *need* to change this.
I understand and partly agree. But it appears that with these changes, the dereference of a null pointer is impossible
evenin builds where assertions are disabled. Previously, this issue could theoretically occur. Consequently, these
changesslightly enhance overall security.
 

-- 
Best regards,
Alexander Kuznetsov