Обсуждение: [PATCH] Fix null pointer dereference in PG19
Hi, I discovered that it's possible to crash Postgres when using VIEWS, FOR PORTION OF syntax and INSTEAD OF triggers together. See crash.sql. This happens because in ExecModifyTable() around line 4827 there is no check for `relkind == RELKIND_VIEW`. If this is the case `tupleid` ends up being NULL which causes null pointer dereference later when ExecDeleteEpilogue() or ExecUpdateEpilogue() calls ExecForPortionOfLeftovers() with tupleid = NULL. An example stacktrace is attached. I propose fixing this by explicitly forbidding using the named features together. See the patch. -- Best regards, Aleksander Alekseev
Вложения
Aleksander Alekseev <aleksander@tigerdata.com> writes:
> I propose fixing this by explicitly forbidding using the named
> features together. See the patch.
Checking this at parse time is completely the wrong thing.
The view could have gained (or lost) triggers by the time
it's executed.
Actually, looking at it, transformForPortionOfClause is quite
full of premature error checks:
* I'd tend to move the anti-FDW check to execution too.
It's not actively wrong, since nowadays we don't permit
relations to change relkind, but it seems out of place.
Also, it seems inadequate to deal with the case of a target
that is a partitioned table having FDW partitions.
* contain_volatile_functions_after_planning is utterly wrong
to apply here. That should happen somewhere in the planner,
where it'd be cheaper as well as not premature.
* I'm inclined to think that pretty much all the mucking with
opclasses is misplaced too: those structures are not immutable
either. All of that should move to rewriting, planning, or
even executor startup.
regards, tom lane
On Tue, Apr 21, 2026 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Aleksander Alekseev <aleksander@tigerdata.com> writes:
> > I propose fixing this by explicitly forbidding using the named
> > features together. See the patch.
>
> Checking this at parse time is completely the wrong thing.
> The view could have gained (or lost) triggers by the time
> it's executed.
>
> Actually, looking at it, transformForPortionOfClause is quite
> full of premature error checks:
>
> * I'd tend to move the anti-FDW check to execution too.
> It's not actively wrong, since nowadays we don't permit
> relations to change relkind, but it seems out of place.
> Also, it seems inadequate to deal with the case of a target
> that is a partitioned table having FDW partitions.
>
> * contain_volatile_functions_after_planning is utterly wrong
> to apply here. That should happen somewhere in the planner,
> where it'd be cheaper as well as not premature.
>
> * I'm inclined to think that pretty much all the mucking with
> opclasses is misplaced too: those structures are not immutable
> either. All of that should move to rewriting, planning, or
> even executor startup.
Thanks for this feedback! I will work on a patch to move those checks
to different phases, and also guard against FDW partitions.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> On Tue, Apr 21, 2026 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * I'm inclined to think that pretty much all the mucking with
>> opclasses is misplaced too: those structures are not immutable
>> either. All of that should move to rewriting, planning, or
>> even executor startup.
> Thanks for this feedback! I will work on a patch to move those checks
> to different phases, and also guard against FDW partitions.
Actually, on second thought that part might be okay. It's not so
different from things we do elsewhere in the parser, such as pick a
suitable equality operator for an IN clause. Even though you could
argue that those decisions ought to be postponed till we're ready to
execute the statement, it's not so bad because what the construct
actually ends up depending on is a specific operator or function.
Even if the user later changes the operator class that we used to find
that function, its semantics are presumably still fit for purpose,
or close enough.
I definitely don't like checking volatility at parse time, though.
(BTW, to be clear: the situation of concern here is where we parse a
query and put it into a rule or new-style SQL function or the like.
By the time we get to executing that parse tree, much might have
changed. Anything we expect to still be there had better be recorded
as a dependency of the parsetree. Even then, the dependency only
guarantees existence of the object, not that its properties didn't
change.)
regards, tom lane
On Tue, Apr 21, 2026 at 4:11 PM Paul A Jungwirth <pj@illuminatedcomputing.com> wrote: > > On Tue, Apr 21, 2026 at 6:41 AM Aleksander Alekseev > <aleksander@tigerdata.com> wrote: > > > > I discovered that it's possible to crash Postgres when using VIEWS, > > FOR PORTION OF syntax and INSTEAD OF triggers together. See crash.sql. > > > > This happens because in ExecModifyTable() around line 4827 there is no > > check for `relkind == RELKIND_VIEW`. If this is the case `tupleid` > > ends up being NULL which causes null pointer dereference later when > > ExecDeleteEpilogue() or ExecUpdateEpilogue() calls > > ExecForPortionOfLeftovers() with tupleid = NULL. An example stacktrace > > is attached. > > Thanks for testing FOR PORTION OF! This specific bug has been reported > already and has a patch at [1]. > > [1] https://www.postgresql.org/message-id/CAHg%2BQDd74fnd4obCRMqVS0AVWf%3DcSFH%3DCv7trTJWgm%2B_bhTK6w%40mail.gmail.com > > > I propose fixing this by explicitly forbidding using the named > > features together. See the patch. > > I don't think disabling these features is necessary. You are right > that we can't use the tupleid when we have a view, but I think an > INSTEAD OF trigger should cause us to skip inserting temporal > leftovers. (If we didn't do the update, we can't draw conclusions > about what history was touched vs untouched.) (Quoting my full message yesterday since I forgot to Reply-All.) Here is v4 of the fix for this. I'd like to continue the conversation here since that other thread is huge and attached to the original commitfest entry for the feature. I'll make a new CF entry for just this thread. This patch squashes jian he's test enhancements from his v3 patch. Also I changed '[2024-01-01,2024-12-31)' to '[2024-01-01,2025-01-01)', which look less like a mistake (not that it matters to the test), and I cleaned up the test comment a little. Yours, -- Paul ~{:-) pj@illuminatedcomputing.com
Вложения
On Wed, Apr 22, 2026 at 12:42 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Here is v4 of the fix for this. I'd like to continue the conversation
> here since that other thread is huge and attached to the original
> commitfest entry for the feature. I'll make a new CF entry for just
> this thread.
>
> This patch squashes jian he's test enhancements from his v3 patch.
> Also I changed '[2024-01-01,2024-12-31)' to '[2024-01-01,2025-01-01)',
> which look less like a mistake (not that it matters to the test), and
> I cleaned up the test comment a little.
Here is a v5, documenting that we skip all FOR PORTION OF work in case
of an INSTEAD OF trigger.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Вложения
On Tue, Apr 21, 2026 at 11:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> * contain_volatile_functions_after_planning is utterly wrong
> to apply here. That should happen somewhere in the planner,
> where it'd be cheaper as well as not premature.
>
typedef struct ForPortionOfExpr
{
NodeTag type;
Var *rangeVar; /* Range column */
char *range_name; /* Range name */
Node *targetFrom; /* FOR PORTION OF FROM bound, if given */
Node *targetTo; /* FOR PORTION OF TO bound, if given */
Node *targetRange; /* FOR PORTION OF bounds as a
range/multirange */
Oid rangeType; /* (base)type of targetRange */
bool isDomain; /* Is rangeVar a domain? */
Node *overlapsExpr; /* range && targetRange */
List *rangeTargetList; /* List of TargetEntrys to set the time
* column(s) */
Oid withoutPortionProc; /* SRF proc for old_range -
target_range */
ParseLoc location; /* token location, or -1 if unknown */
ParseLoc targetLocation; /* token location, or -1 if unknown */
} ForPortionOfExpr;
targetFrom and targetTo is only for deparsing purpose, skip
eval_const_expressions should be fine.
RewriteQuery, we have:
``````
AddQual(parsetree, parsetree->forPortionOf->overlapsExpr);
/* Update FOR PORTION OF column(s) automatically. */
foreach(tl, parsetree->forPortionOf->rangeTargetList)
{
TargetEntry *tle = (TargetEntry *) lfirst(tl);
parsetree->targetList = lappend(parsetree->targetList, tle);
}
``````
rangeTargetList and overlapsExpr will go through eval_const_expressions.
Only ForPortionOfExpr->targetRange really needs to be dealt with.
In ExecInitModifyTable, we already did
ExecPrepareExpr(forPortionOf->targetRange),
which will do eval_const_expressions(forPortionOf->targetRange).
moving contain_volatile_functions_after_planning to
ExecInitModifyTable should be fine.
In ExecInitModifyTable, we can't just
```
exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate);
if (contain_volatile_functions_after_planning(exprState->expr)
```
Because of the comments below in execnodes.h:
typedef struct ExprState
/* original expression tree, for debugging only */
Expr *expr;
While at it, add errcode to the surrounding code.
--
jian
https://www.enterprisedb.com/
Вложения
On Tue, Apr 21, 2026 at 11:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > * I'd tend to move the anti-FDW check to execution too. > It's not actively wrong, since nowadays we don't permit > relations to change relkind, but it seems out of place. > Also, it seems inadequate to deal with the case of a target > that is a partitioned table having FDW partitions. > Hi. Instead of adding another subnode in CheckValidResultRel, I am passing ModifyTable to it, this will be more future-proof. -- jian https://www.enterprisedb.com/