Обсуждение: [BUG FIX] Uninitialized var fargtypes used.
Hi,
Can anyone check this bug fix?
Thanks.
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\commands\event_trigger.c Mon Sep 30 17:06:55 2019
+++ event_trigger.c Mon Nov 11 13:52:35 2019
@@ -171,7 +171,7 @@
HeapTuple tuple;
Oid funcoid;
Oid funcrettype;
- Oid fargtypes[1]; /* dummy */
+ Oid fargtypes[1] = {InvalidOid, InvalidOid}; /* dummy */
Oid evtowner = GetUserId();
ListCell *lc;
List *tags = NULL;
Вложения
Ranier Vilela <ranier_gyn@hotmail.com> writes:
> Can anyone check this bug fix?
> - Oid fargtypes[1]; /* dummy */
> + Oid fargtypes[1] = {InvalidOid, InvalidOid}; /* dummy */
Well, it's wrong on its face, because that array only has one element
not two. But why do you care? The element will never be accessed.
The only reason we declare this variable at all is that LookupFuncName
requires a non-null pointer, which if memory serves is because memcmp()
with a null pointer is formally undefined even if the count is zero,
cf commit 0a52d378b.
Maybe it would've been better to make LookupFuncName deal with the
case instead of requiring callers to do strange things. But I don't
see any bug here.
regards, tom lane
At Mon, 11 Nov 2019 12:32:38 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> Ranier Vilela <ranier_gyn@hotmail.com> writes:
> > Can anyone check this bug fix?
>
> > - Oid fargtypes[1]; /* dummy */
> > + Oid fargtypes[1] = {InvalidOid, InvalidOid}; /* dummy */
>
> Well, it's wrong on its face, because that array only has one element
> not two. But why do you care? The element will never be accessed.
>
> The only reason we declare this variable at all is that LookupFuncName
> requires a non-null pointer, which if memory serves is because memcmp()
> with a null pointer is formally undefined even if the count is zero,
> cf commit 0a52d378b.
Yes, what is needed there is a valid pointer with any content.
> Maybe it would've been better to make LookupFuncName deal with the
> case instead of requiring callers to do strange things. But I don't
> see any bug here.
Actually it's not an actual bug but a cosmetic adjustment, but not
that bad, I think. Requiring dummy pointer is already a strange thing.
By the way looking closer the function, IIUC, very small change can do
that.
> /*
> * If no arguments were specified, the name must yield a unique candidate.
> */
> if (nargs < 0)
> {
We can change the condition with "nargs <= 0" and it should return the
only element in clist. If the catalog is broken we may get
FUNCLOOKUP_AMBIGUOUS but it seems rather the correct behavior.
This allows argtypes == NULL and makes the caller-side tweak useless.
Thoughts?
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 8e926539e6..1686a80403 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2035,9 +2035,6 @@ LookupFuncNameInternal(List *funcname, int nargs, const Oid *argtypes,
{
FuncCandidateList clist;
- /* Passing NULL for argtypes is no longer allowed */
- Assert(argtypes);
-
/* Always set *lookupError, to forestall uninitialized-variable warnings */
*lookupError = FUNCLOOKUP_NOSUCHFUNC;
@@ -2047,7 +2044,7 @@ LookupFuncNameInternal(List *funcname, int nargs, const Oid *argtypes,
/*
* If no arguments were specified, the name must yield a unique candidate.
*/
- if (nargs < 0)
+ if (nargs <= 0)
{
if (clist)
{
@@ -2064,6 +2061,9 @@ LookupFuncNameInternal(List *funcname, int nargs, const Oid *argtypes,
return InvalidOid;
}
+ /* Passing NULL for argtypes is no longer allowed */
+ Assert(argtypes);
+
/*
* Otherwise, look for a match to the arg types. FuncnameGetCandidates
* has ensured that there's at most one match in the returned list.
On 2019-Nov-12, Kyotaro Horiguchi wrote:
> At Mon, 11 Nov 2019 12:32:38 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > /*
> > * If no arguments were specified, the name must yield a unique candidate.
> > */
> > if (nargs < 0)
> > {
>
> We can change the condition with "nargs <= 0" and it should return the
> only element in clist. If the catalog is broken we may get
> FUNCLOOKUP_AMBIGUOUS but it seems rather the correct behavior.
>
> This allows argtypes == NULL and makes the caller-side tweak useless.
Yeah, essentially this reverts 0a52d378b03b. Here's another version of
this I wrote last night.
BTW I also noticed that ProcedureCreate forces its callers to pass a
non-NULL parameterTypes even for 0 args, which seems to be fixed easily
too, something like this:
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index ef009ad2bc..547949e534 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -124,7 +124,7 @@ ProcedureCreate(const char *procedureName,
*/
Assert(PointerIsValid(prosrc));
- parameterCount = parameterTypes->dim1;
+ parameterCount = parameterTypes == NULL ? 0 : parameterTypes->dim1;
if (parameterCount < 0 || parameterCount > FUNC_MAX_ARGS)
ereport(ERROR,
(errcode(ERRCODE_TOO_MANY_ARGUMENTS),
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Nov-12, Kyotaro Horiguchi wrote:
>> We can change the condition with "nargs <= 0" and it should return the
>> only element in clist. If the catalog is broken we may get
>> FUNCLOOKUP_AMBIGUOUS but it seems rather the correct behavior.
> Yeah, essentially this reverts 0a52d378b03b. Here's another version of
> this I wrote last night.
I really dislike the s/nargs < 0/nargs <= 0/ aspect of these proposals.
That's conflating two fundamentally different corner cases. We have
(1) for the nargs<0 case, there must not be more than one match,
else it's a user mistake (and not an unlikely one).
(2) for the nargs==0 case, if there's more than one match, we either
have corrupted catalogs or some kind of software bug.
The argument for changing the code like this seems to be "we'll
assume the possibility of a bug/corruption is so small that it's
okay to treat it as a user mistake". I reject that line of thinking.
And I especially reject conflating the two cases without bothering
to fix the adjacent comment that describes only case 1.
If we're going to revert 0a52d378b03b we should just treat the
problem straightforwardly. I was imagining
- if (memcmp(argtypes, clist->args, nargs * sizeof(Oid)) == 0)
+ /* if nargs==0, argtypes can be null; don't pass that to memcmp */
+ if (nargs == 0 ||
+ memcmp(argtypes, clist->args, nargs * sizeof(Oid)) == 0)
It's really stretching credulity to imagine that one more test-and-branch
in this loop costs anything worth noticing, especially compared to the
costs of having built the list to begin with. So I'm now feeling that
0a52d378b03b was penny-wise and pound-foolish.
Or, in other words: the OP's complaint here is basically that the
existing code isn't being straightforward about how it handles this
scenario. Changing to a different non-straightforward fix isn't
much of an improvement.
regards, tom lane
On 2019-Nov-12, Tom Lane wrote: > If we're going to revert 0a52d378b03b we should just treat the > problem straightforwardly. I was imagining > > - if (memcmp(argtypes, clist->args, nargs * sizeof(Oid)) == 0) > + /* if nargs==0, argtypes can be null; don't pass that to memcmp */ > + if (nargs == 0 || > + memcmp(argtypes, clist->args, nargs * sizeof(Oid)) == 0) > > It's really stretching credulity to imagine that one more test-and-branch > in this loop costs anything worth noticing, especially compared to the > costs of having built the list to begin with. So I'm now feeling that > 0a52d378b03b was penny-wise and pound-foolish. I pushed using that approach. For a minute I thought that it should also test that clist->nargs also equals 0, but that turns out not to be necessary since the number of arguments is already considered by FuncnameGetCandidates above. Thanks -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Nov-12, Tom Lane wrote:
>> If we're going to revert 0a52d378b03b we should just treat the
>> problem straightforwardly. I was imagining ...
> I pushed using that approach.
You missed cleaning up func_get_detail, but I took care of it.
regards, tom lane
At Tue, 12 Nov 2019 11:07:10 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in > The argument for changing the code like this seems to be "we'll > assume the possibility of a bug/corruption is so small that it's > okay to treat it as a user mistake". I reject that line of thinking. Ah, understood. > And I especially reject conflating the two cases without bothering > to fix the adjacent comment that describes only case 1. Mm. I should be more careful.. Anyway thanks for fixing this! regards. -- Kyotaro Horiguchi NTT Open Source Software Center