Detect buffer underflow in get_th()
Hello everyone,
In src/backend/utils/adt/formatting.c:1516, there is a get_th() function
utilized to return ST/ND/RD/TH suffixes for simple numbers.
Upon reviewing its behavior, it appears capable of receiving non-numeric inputs
(this is verified by a check at formatting.c:1527).
Given that the function can accept non-numeric inputs,
it is plausible that it could also receive an empty input,
although a brief examination of its calls did not reveal any such instances.
Nevertheless, if the function were to receive an empty input of zero length,
a buffer underflow would occur when attempting to compute *(num + (len - 1)),
as (len - 1) would result in a negative shift.
To mitigate this issue, I propose a patch incorporating the
zero_length_character_string error code, as detailed in the attachment.
--
Best regards,
Alexander KuznetsovFrom de36780802b72b643b47ec129979292e9832e9e7 Mon Sep 17 00:00:00 2001
From: Alexander Kuznetsov
Date: Wed, 24 Jul 2024 12:31:45 +0300
Subject: [PATCH] Detect buffer underflow in get_th()
If get_th() can receive input that is not a number,
then it can also receive empty input.
Empty input with zero length can result in a buffer underflow
when accessing *(num + (len - 1)), as (len - 1) would produce a negative index.
Add a check for zero-length input to prevent it.
This was found by ALT Linux Team.
---
src/backend/utils/adt/formatting.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 8736ada4be..0ab5ac5bf0 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -1518,6 +1518,11 @@ get_th(char *num, int type)
int len = strlen(num),
last;
+ if (len == 0)
+ ereport(ERROR,
+(errcode(ERRCODE_ZERO_LENGTH_CHARACTER_STRING),
+ errmsg("input cannot be empty string")));
+
last = *(num + (len - 1));
if (!isdigit((unsigned char) last))
ereport(ERROR,
--
2.42.2
Re: Detect buffer underflow in get_th()
24.07.2024 18:39, Peter Eisentraut wrote: If it can't happen in practice, maybe an assertion would be enough? In practice, the function should not receive non-numeric strings either; however, since there is an exception in place for such cases, I thought it would be good to add a check for zero-length input in a similar manner. But of course it's open for discussion and team decision whether this should be addressed as an assertion or handled differently. -- Best regards, Alexander Kuznetsov
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 KuznetsovFrom acabe34b714a9c311bfb85e5be94e6fe906fa9f1 Mon Sep 17 00:00:00 2001
From: Alexander Kuznetsov
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
Re: Possible null pointer dereference in afterTriggerAddEvent()
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
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
PostgreSQL's approach to assertion usage: seeking best practices
Hello everyone, The ALT Linux Team has recently initiated a focused testing effort on PostgreSQL, with an emphasis on its security aspects. As part of this effort, we conducted static analysis using Svace, which raised some questions regarding the use of the Assert() function. We were unable to find clear answers in the documentation or previous discussions and would greatly appreciate your insights, as these will influence how we approach future patch submissions: 1. General purpose of Assert() function: - Is the primary purpose of the Assert() function to: - Inform developers of the assumptions made in the code, - Facilitate testing of new builds with assertions enabled, - Accelerate development by temporarily placing assertions where defensive code may later be required, - Or does it serve some other purpose? 2. Assertions and defensive code: We have observed patches where assertions were added to defensive code (e.g., [1]) and where defensive code was added to assertions. Is it generally advisable and encouraged to incorporate defensive code into assertions' assumptions? For instance, we encountered cases where a null pointer dereference occurs immediately after an assertion that the pointer is not null. In assertion-enabled builds, this results in an assertion failure, while in non-assert builds, it leads to a dereference, which we aim to avoid. However, it is unclear to us whether the use of an assertion in such cases indicates that this flaw is known and will not be addressed specifically, or if additional protective code should be introduced. A clearer understanding of whether assertions should signal potential failure points that might later be rewritten or protected would assist us in quickly developing fixes for such cases, particularly where we believe the issue could occur in practice. 3. Approach to fixing flaws: How should we generally fix the flaws - by adding protection code, or by adding assertions? I previously submitted two patches and encountered some confusion based on the feedback: in one instance, I added protective code but was asked whether an assertion would suffice [2], and in another, I added an assertion but was questioned on its necessity, given that it would cause a failure regardless, which I agreed with [3]. Your guidance on these matters would be invaluable in helping us align our patch submissions with the community’s best practices. Thank you in advance for your time and assistance. 1. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=30aaab26e52144097a1a5bbb0bb66ea1ebc0cb81 2. https://www.postgresql.org/message-id/e22df993-cdb4-4d0a-b629-42211ebed582%40altlinux.org 3. https://www.postgresql.org/message-id/6d0323c3-3f5d-4137-af73-98a5ab90e77c%40altlinux.org -- Best regards, Alexander Kuznetsov
Check for tuplestorestate nullness before dereferencing
Hello everyone, I'd like to propose adding a check for the nullness of tuplestorestate before dereferencing it in src/backend/executor/nodeModifier.c. The patch is attached. I am proposing this fix based on the assumption that tuplestorestate could be NULL since there is a check for it when calculating eof_tuplestore at line 85. However, since this code hasn't been changed since 2006 and hasn't caused any issues, it is possible that the check for (tuplestorestate == NULL) is redundant when calculating eof_tuplestore. -- Best regards, Alexander Kuznetsov From b5217fd138f35fb5bf70ad8741ebed5330457891 Mon Sep 17 00:00:00 2001 From: Alexander Kuznetsov Date: Thu, 10 Oct 2024 17:38:10 +0300 Subject: [PATCH] Check for tuplestorestate nullness before dereferencing tuplestorestate can be NULL when calculating eof_tuplestore, where tuplestorestate is dereferenced by tuplestore_gettuple(). Add check for nullness before dereferencing. Found by ALT Linux Team with Svace. --- src/backend/executor/nodeMaterial.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c index 22e1787fbd..5bc8561f3a 100644 --- a/src/backend/executor/nodeMaterial.c +++ b/src/backend/executor/nodeMaterial.c @@ -95,7 +95,7 @@ ExecMaterial(PlanState *pstate) * to return the one before that, if possible. So do an extra * fetch. */ - if (!tuplestore_advance(tuplestorestate, forward)) + if (tuplestorestate == NULL || !tuplestore_advance(tuplestorestate, forward)) return NULL; /* the tuplestore must be empty */ } eof_tuplestore = false; -- 2.42.2
Re: Check for tuplestorestate nullness before dereferencing
14.10.2024 19:21, Alena Rybakina wrote: To be honest, I'm not sure this change is necessary. Looking at the code, I see that in ExecMaterial it is possible to handle a tuplestorestate of NULL, and this error can be accessed if the flags are not zero, but I think these cases have been worked out. [...] After the subplan scan is complete, we see that the eof_underlying variable becomes true, and this part of the code will no longer be accessible. tuplestorestate also becomes Null. I also noticed that tuplestorestate=NULL is an indicator that the scan is complete, so if this section of code is called, something is wrong earlier in the code. It appears to me that prior to the earlier commit [1], tuplestorestate was always initialized if it was NULL. In that commit, flags were introduced, allowing for the possibility that tuplestorestate could remain NULL further along in the code. This is why I believe that checks for its nullness were added before calling dereferencing functions, such as the check in line 146, which is still present. However, the tuplestore_getheaptuple() function, which is no longer present, remained unchanged until commit [2], where it was replaced with tuplestore_gettupleslot() and tuplestore_advance(). While it was acceptable in case of tuplestore_gettupleslot() that can handle a NULL tuplestorestate, tuplestore_advance() requires tuplestorestate to not be NULL when it is called. This leads to my confusion as to why there is no check for the nullness of tuplestorestate before calling tuplestore_advance(). [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d2c555ee538f34be7aff744b994df4d2369a9140 [2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3f50ba27cf417eb57fd310c2a88f76a6ea6b966e -- Best regards, Alexander Kuznetsov
Re: [PATCH] Check for TupleTableSlot nullness before dereferencing
03.10.2024 12:48, Daniel Gustafsson wrote: From a quick reading we can only reach there after evaluating an expression, so can it really be null though? This code hasn't changed all that much since 2009, if there was a reachable segfault on a null pointer deref I have a feeling we'd heard about it by now so some extra care seems warranted to ensure it's not a static analyzer false positive. Thanks for your response! It seems to me that dereferencing is possible under the following scenario: 1. slot2 is NULL at line 968, 2. The while loop at line 971 executes once, filling slot1 (slot2 still remains NULL), 3. No changes occur to slot2 thereafter, up to line 1003, 4. At line 1003, slot2 is swapped with slot1 (assuming numDistinctCols > 0), 5. At line 1016, slot1 is dereferenced with conent of slot2 (NULL). This entire reasoning is based on the assumption that slot2 can theoretically be NULL, as there is such a check at line 968. Is it possible that no errors have occurred because this condition has always been satisfied and is, perhaps, redundant, or maybe I'm misunderstanding something? -- Best regards, Alexander Kuznetsov
Re: Detect buffer underflow in get_th()
Hello, is there anything else we can help with or discuss in order to apply this fix? 24.07.2024 18:53, Alexander Kuznetsov пишет: 24.07.2024 18:39, Peter Eisentraut wrote: If it can't happen in practice, maybe an assertion would be enough? In practice, the function should not receive non-numeric strings either; however, since there is an exception in place for such cases, I thought it would be good to add a check for zero-length input in a similar manner. But of course it's open for discussion and team decision whether this should be addressed as an assertion or handled differently. -- Best regards, Alexander Kuznetsov
Re: Possible null pointer dereference in afterTriggerAddEvent()
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
[PATCH] Check for TupleTableSlot nullness before dereferencing
Hello everyone, I'd like to propose adding check for nullness of TupleTableSlot before dereferencing it in /src/backend/executor/nodeAgg.c It is done in the same manner other TupleTableSlots are checked, but was probably left unseen because slot1 and slot2 variables can be swapped during function execution. The patch is attached. -- Best regards, Alexander Kuznetsov From f490d485e3dbdfec7c6804bd96ae47b5a60d7c96 Mon Sep 17 00:00:00 2001 From: Alexander Kuznetsov Date: Thu, 3 Oct 2024 10:24:08 +0300 Subject: [PATCH] Check for TupleTableSlot nullness before dereferencing At the beginning of process_ordered_aggregate_multi() slot1 is assumed to not be NULL, while slot2 can be NULL. Later, if (numDistinctCols > 0), slot1 and slot2 are swapped, and slot1 (with possible contents of slot2) is dereferenced by ExecClearTuple(). Add check for nullness before dereferencing. Found by ALT Linux Team with Svace. --- src/backend/executor/nodeAgg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index 53ead77ece..26e7938a47 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -1013,7 +1013,8 @@ process_ordered_aggregate_multi(AggState *aggstate, /* Reset context each time */ ResetExprContext(tmpcontext); - ExecClearTuple(slot1); + if (slot1) + ExecClearTuple(slot1); } if (slot2) -- 2.42.2
Re: [PATCH] Check for TupleTableSlot nullness before dereferencing
Hello, ping. What do you think about reasoning below? Maybe we should consider proposing different patch for removing redundant check there? 09.10.2024 18:23, Alexander Kuznetsov wrote: 03.10.2024 12:48, Daniel Gustafsson wrote: From a quick reading we can only reach there after evaluating an expression, so can it really be null though? This code hasn't changed all that much since 2009, if there was a reachable segfault on a null pointer deref I have a feeling we'd heard about it by now so some extra care seems warranted to ensure it's not a static analyzer false positive. Thanks for your response! It seems to me that dereferencing is possible under the following scenario: [...] This entire reasoning is based on the assumption that slot2 can theoretically be NULL, as there is such a check at line 968. Is it possible that no errors have occurred because this condition has always been satisfied and is, perhaps, redundant, or maybe I'm misunderstanding something? -- Best regards, Alexander Kuznetsov
Re: Detect buffer underflow in get_th()
Hello, ping? 24.09.2024 17:52, Alexander Kuznetsov wrote: Hello, is there anything else we can help with or discuss in order to apply this fix? 24.07.2024 18:53, Alexander Kuznetsov пишет: 24.07.2024 18:39, Peter Eisentraut wrote: If it can't happen in practice, maybe an assertion would be enough? In practice, the function should not receive non-numeric strings either; however, since there is an exception in place for such cases, I thought it would be good to add a check for zero-length input in a similar manner. But of course it's open for discussion and team decision whether this should be addressed as an assertion or handled differently. -- Best regards, Alexander Kuznetsov
pg_dump: Fix dangling pointer in EndCompressorZstd()
Hello everyone, We've found that EndCompressorZstd() doesn't set cs->private_data to NULL after pg_free(), unlike other EndCompressor implementations. While this doesn't currently cause issues (as the pointer soon gets reassigned), we recommend fixing this to maintain consistency with other implementations and prevent potential future issues. The patch is attached, would appreciate your thoughts on this change. -- Best regards, Alexander Kuznetsov From 428c60888f96aa5d0b7575a4342cdce4ff0257ab Mon Sep 17 00:00:00 2001 From: Alexander Kuznetsov Date: Wed, 16 Apr 2025 11:19:56 +0300 Subject: [PATCH] pg_dump: Fix dangling pointer in EndCompressorZstd() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cs->private_data becomes dangling after pg_free() call and should be set to NULL (consistent with other EndCompressor implementations) Found by Linux Verification Center (linuxtesting.org) with Svace. --- src/bin/pg_dump/compress_zstd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bin/pg_dump/compress_zstd.c b/src/bin/pg_dump/compress_zstd.c index 1f7b4942706..cb595b10c2d 100644 --- a/src/bin/pg_dump/compress_zstd.c +++ b/src/bin/pg_dump/compress_zstd.c @@ -142,6 +142,7 @@ EndCompressorZstd(ArchiveHandle *AH, CompressorState *cs) /* output buffer may be allocated in either mode */ pg_free(zstdcs->output.dst); pg_free(zstdcs); + cs->private_data = NULL; } static void -- 2.42.4
