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 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.
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
Attachments:
0001-Add-assertion-of-an-empty-list-in-afterTriggerAddEve.patchtext/x-patch; charset=UTF-8; name=0001-Add-assertion-of-an-empty-list-in-afterTriggerAddEve.patchDownload
From acabe34b714a9c311bfb85e5be94e6fe906fa9f1 Mon Sep 17 00:00:00 2001
From: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Date: Thu, 25 Jul 2024 16:24:18 +0300
Subject: [PATCH] Add assertion of an empty list in afterTriggerAddEvent()
It is possible for events->tail to still be NULL at this point,
so assert it's not NULL before dereferencing.
This was found by ALT Linux Team with Svace.
---
src/backend/commands/trigger.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 170360edda..a0946025a5 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -4099,7 +4099,10 @@ afterTriggerAddEvent(AfterTriggerEventList *events,
if (events->head == NULL)
events->head = chunk;
else
+ {
+ Assert(events->tail != NULL);
events->tail->next = chunk;
+ }
events->tail = chunk;
/* events->tailfree is now out of sync, but we'll fix it below */
}
--
2.42.2
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 even in builds where assertions are disabled. Previously, this issue could theoretically occur. Consequently, these changes slightly enhance overall security.
--
Best regards,
Alexander Kuznetsov
Attachments:
v2-0001-Add-assertion-of-an-empty-list-in-afterTriggerAdd.patchtext/x-patch; charset=UTF-8; name=v2-0001-Add-assertion-of-an-empty-list-in-afterTriggerAdd.patchDownload
From 63a3a19fe67bfd1f427b21d53ff0ef642aed89c4 Mon Sep 17 00:00:00 2001
From: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Date: Fri, 26 Jul 2024 11:55:53 +0300
Subject: [PATCH v2] Add assertion of an empty list in afterTriggerAddEvent()
The unwritten assumption is that events->tail and events->head are either
both NULL or neither is NULL.
Change the order of checks to ensure that we never try
to dereference events->tail if it is NULL.
This was found by ALT Linux Team with Svace.
---
src/backend/commands/trigger.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 170360edda..e039d62b0b 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -4096,8 +4096,11 @@ afterTriggerAddEvent(AfterTriggerEventList *events,
chunk->endptr = chunk->endfree = (char *) chunk + chunksize;
Assert(chunk->endfree - chunk->freeptr >= needed);
- if (events->head == NULL)
+ if (events->tail == NULL)
+ {
+ Assert(events->head == NULL);
events->head = chunk;
+ }
else
events->tail->next = chunk;
events->tail = chunk;
--
2.42.2
Hello,
is there anything else we can help with or discuss in order to apply this fix?
26.07.2024 12:16, Alexander Kuznetsov пишет:
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 even in builds where assertions are disabled. Previously, this issue could theoretically occur. Consequently, these changes slightly enhance overall security.
--
Best regards,
Alexander Kuznetsov
On 2024-Sep-24, Alexander Kuznetsov wrote:
Hello,
is there anything else we can help with or discuss in order to apply this fix?
I don't think so, it seems a no-brainer to me and there are no
objections. I'll get it pushed tomorrow.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)
On 2024-Sep-24, Alvaro Herrera wrote:
On 2024-Sep-24, Alexander Kuznetsov wrote:
is there anything else we can help with or discuss in order to apply this fix?
I don't think so, it seems a no-brainer to me and there are no
objections. I'll get it pushed tomorrow.
OK, done.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"The Postgresql hackers have what I call a "NASA space shot" mentality.
Quite refreshing in a world of "weekend drag racer" developers."
(Scott Marlowe)