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
From 63a3a19fe67bfd1f427b21d53ff0ef642aed89c4 Mon Sep 17 00:00:00 2001
From: Alexander Kuznetsov <kuznetso...@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
