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 <[email protected]> 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
