Thread: Explicit NULL dereference (src/backend/utils/adt/ruleutils.c)
Hi,
Per Coverity.
make_ruledef function can dereference a NULL pointer (actions),
if "ev_qual" is provided and "actions" does not exist.
The comment there is contradictory: " /* these could be nulls */ "
Because if "ev_qual" is not null, "actions" cannot be either.
Because if "ev_qual" is not null, "actions" cannot be either.
Solution proposed merely as a learning experience.
regards,
Ranier Vilela
Attachment
At Sat, 31 Oct 2020 11:49:07 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in > Per Coverity. > > make_ruledef function can dereference a NULL pointer (actions), > if "ev_qual" is provided and "actions" does not exist. > > The comment there is contradictory: " /* these could be nulls */ " > Because if "ev_qual" is not null, "actions" cannot be either. > > Solution proposed merely as a learning experience. We cannot reach there with ev_action == NULL since it comes from a non-nullable column. Since most of the other columns has an assertion that !isnull, I think we should do the same thing for ev_action (and ev_qual). SPI_getvalue() returns C-NULL for SQL-NULL (or for some other unexpected situations.). regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 6c656586e8..6ba2b4a5ae 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -4766,12 +4766,13 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, /* these could be nulls */ fno = SPI_fnumber(rulettc, "ev_qual"); ev_qual = SPI_getvalue(ruletup, rulettc, fno); + Assert(ev_qual != NULL); fno = SPI_fnumber(rulettc, "ev_action"); ev_action = SPI_getvalue(ruletup, rulettc, fno); - if (ev_action != NULL) - actions = (List *) stringToNode(ev_action); - + Assert(ev_action != NULL); + actions = (List *) stringToNode(ev_action); + ev_relation = table_open(ev_class, AccessShareLock); /*
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > We cannot reach there with ev_action == NULL since it comes from a > non-nullable column. Since most of the other columns has an assertion > that !isnull, I think we should do the same thing for ev_action (and > ev_qual). SPI_getvalue() returns C-NULL for SQL-NULL (or for some > other unexpected situations.). Isn't the comment just above there wrong? /* these could be nulls */ I wonder just when that became outdated. regards, tom lane
At Mon, 02 Nov 2020 10:36:10 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Sat, 31 Oct 2020 11:49:07 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in > > Per Coverity. > > > > make_ruledef function can dereference a NULL pointer (actions), > > if "ev_qual" is provided and "actions" does not exist. > > > > The comment there is contradictory: " /* these could be nulls */ " > > Because if "ev_qual" is not null, "actions" cannot be either. > > > > Solution proposed merely as a learning experience. > > We cannot reach there with ev_action == NULL since it comes from a > non-nullable column. Since most of the other columns has an assertion > that !isnull, I think we should do the same thing for ev_action (and > ev_qual). SPI_getvalue() returns C-NULL for SQL-NULL (or for some > other unexpected situations.). The following code is found there, since 1998. (15cb32d93e) > /* If the rule has an event qualification, add it */ > if (ev_qual == NULL) > ev_qual = ""; The problem code here was written as the follows. + fno = SPI_fnumber(rulettc, "is_instead"); + is_instead = (bool)SPI_getbinval(ruletup, rulettc, fno, &isnull); + + fno = SPI_fnumber(rulettc, "ev_qual"); + ev_qual = SPI_getvalue(ruletup, rulettc, fno); + if (isnull) ev_qual = NULL; + + fno = SPI_fnumber(rulettc, "ev_action"); + ev_action = SPI_getvalue(ruletup, rulettc, fno); + if (isnull) ev_action = NULL; + if (ev_action != NULL) { + actions = (List *)stringToNode(ev_action); + } I'm not sure what the code means by just reading there but at least it seems impossible for the current code to return NULL for legit values. regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 6c656586e8..ab4f102e9b 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -4766,12 +4766,13 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, /* these could be nulls */ fno = SPI_fnumber(rulettc, "ev_qual"); ev_qual = SPI_getvalue(ruletup, rulettc, fno); + Assert(ev_qual != NULL); fno = SPI_fnumber(rulettc, "ev_action"); ev_action = SPI_getvalue(ruletup, rulettc, fno); - if (ev_action != NULL) - actions = (List *) stringToNode(ev_action); - + Assert(ev_action != NULL); + actions = (List *) stringToNode(ev_action); + ev_relation = table_open(ev_class, AccessShareLock); /* @@ -4819,9 +4820,6 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, generate_relation_name(ev_class, NIL) : generate_qualified_relation_name(ev_class)); - /* If the rule has an event qualification, add it */ - if (ev_qual == NULL) - ev_qual = ""; if (strlen(ev_qual) > 0 && strcmp(ev_qual, "<>") != 0) { Node *qual;
At Sun, 01 Nov 2020 21:05:29 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > > We cannot reach there with ev_action == NULL since it comes from a > > non-nullable column. Since most of the other columns has an assertion > > that !isnull, I think we should do the same thing for ev_action (and > > ev_qual). SPI_getvalue() returns C-NULL for SQL-NULL (or for some > > other unexpected situations.). > > Isn't the comment just above there wrong? > > /* these could be nulls */ > > I wonder just when that became outdated. Mmm. I investigated that. At the very beginning of CREATE RULE (d31084e9d1, 1996), InsertRule() did the following. > template = "INSERT INTO pg_rewrite \ >(rulename, ev_type, ev_class, ev_attr, action, ev_qual, is_instead) VALUES \ >('%s', %d::char, %d::oid, %d::int2, '%s'::text, '%s'::text, \ > '%s'::bool);"; > if (strlen(template) + strlen(rulname) + strlen(actionbuf) + > strlen(qualbuf) + 20 /* fudge fac */ > RULE_PLAN_SIZE) { > elog(WARN, "DefineQueryRewrite: rule plan string too big."); > } > sprintf(rulebuf, template, > rulname, evtype, eventrel_oid, evslot_index, actionbuf, > qualbuf, is_instead); Doesn't seem that ev_qual and ev_action can be NULL. The same function in the current converts action list to string using nodeToSTring so NIL is converted into '<>', which is not NULL. So I think ev_action cannot be null from the beginning of the history unless the columns is modified manually. ev_qual and ev_action are marked as non-nullable (9b39b799db, in 2018). They could be null if we modified that columns nullable then set NULL, but that could happen on all other columns in pg_rewite catalog, which are Assert(!null)ed. Although ev_action cannot be a empty list using SQL interface. So we can get rid of the case list_length(action) == 0, but I'm not sure it's worth doing (but the attaches does..). regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 6c656586e8..120b8869ea 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -4763,15 +4763,15 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, Assert(!isnull); is_instead = DatumGetBool(dat); - /* these could be nulls */ fno = SPI_fnumber(rulettc, "ev_qual"); ev_qual = SPI_getvalue(ruletup, rulettc, fno); + Assert(ev_qual != NULL); fno = SPI_fnumber(rulettc, "ev_action"); ev_action = SPI_getvalue(ruletup, rulettc, fno); - if (ev_action != NULL) - actions = (List *) stringToNode(ev_action); - + Assert(ev_action != NULL); + actions = (List *) stringToNode(ev_action); + ev_relation = table_open(ev_class, AccessShareLock); /* @@ -4819,10 +4819,8 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, generate_relation_name(ev_class, NIL) : generate_qualified_relation_name(ev_class)); - /* If the rule has an event qualification, add it */ - if (ev_qual == NULL) - ev_qual = ""; - if (strlen(ev_qual) > 0 && strcmp(ev_qual, "<>") != 0) + Assert(strlen(ev_qual) > 0); + if (strcmp(ev_qual, "<>") != 0) { Node *qual; Query *query; @@ -4875,6 +4873,7 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, appendStringInfoString(buf, "INSTEAD "); /* Finally the rules actions */ + Assert(list_length(actions) > 0); if (list_length(actions) > 1) { ListCell *action; @@ -4893,10 +4892,6 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, } appendStringInfoString(buf, ");"); } - else if (list_length(actions) == 0) - { - appendStringInfoString(buf, "NOTHING;"); - } else { Query *query;
Em seg., 2 de nov. de 2020 às 01:36, Kyotaro Horiguchi <horikyota.ntt@gmail.com> escreveu:
At Sun, 01 Nov 2020 21:05:29 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > We cannot reach there with ev_action == NULL since it comes from a
> > non-nullable column. Since most of the other columns has an assertion
> > that !isnull, I think we should do the same thing for ev_action (and
> > ev_qual). SPI_getvalue() returns C-NULL for SQL-NULL (or for some
> > other unexpected situations.).
>
> Isn't the comment just above there wrong?
>
> /* these could be nulls */
>
> I wonder just when that became outdated.
Mmm. I investigated that.
At the very beginning of CREATE RULE (d31084e9d1, 1996), InsertRule()
did the following.
> template = "INSERT INTO pg_rewrite \
>(rulename, ev_type, ev_class, ev_attr, action, ev_qual, is_instead) VALUES \
>('%s', %d::char, %d::oid, %d::int2, '%s'::text, '%s'::text, \
> '%s'::bool);";
> if (strlen(template) + strlen(rulname) + strlen(actionbuf) +
> strlen(qualbuf) + 20 /* fudge fac */ > RULE_PLAN_SIZE) {
> elog(WARN, "DefineQueryRewrite: rule plan string too big.");
> }
> sprintf(rulebuf, template,
> rulname, evtype, eventrel_oid, evslot_index, actionbuf,
> qualbuf, is_instead);
Doesn't seem that ev_qual and ev_action can be NULL. The same
function in the current converts action list to string using
nodeToSTring so NIL is converted into '<>', which is not NULL.
So I think ev_action cannot be null from the beginning of the history
unless the columns is modified manually. ev_qual and ev_action are
marked as non-nullable (9b39b799db, in 2018). They could be null if we
modified that columns nullable then set NULL, but that could happen on
all other columns in pg_rewite catalog, which are Assert(!null)ed.
Although ev_action cannot be a empty list using SQL interface. So we
can get rid of the case list_length(action) == 0, but I'm not sure
it's worth doing (but the attaches does..).
I think that Assert is not the right solution here.
For a function that returns NULL twice (SPI_getvalue), it is worth testing the result against NULL.
In the future, any modification may cause further dereference.
In addition, the static analysis tools would continue to note this snippet either as a bug or as a suspect.
In the future, any modification may cause further dereference.
In addition, the static analysis tools would continue to note this snippet either as a bug or as a suspect.
Checking "actions" pointer against NULL, and acting appropriately would do.
regards,
Ranier Vilela
Ranier Vilela <ranier.vf@gmail.com> writes: > Em seg., 2 de nov. de 2020 às 01:36, Kyotaro Horiguchi < > horikyota.ntt@gmail.com> escreveu: >> Doesn't seem that ev_qual and ev_action can be NULL. The same >> function in the current converts action list to string using >> nodeToSTring so NIL is converted into '<>', which is not NULL. > I think that Assert is not the right solution here. I think there's some confusion here: whether the ev_actions column can contain a SQL NULL is a very different thing from whether the result of stringToNode() on it can be a NIL list. The latter should also not happen, but it's not enforced by low-level code in the same way that the NOT NULL property is. So in my judgment, it's okay for us to just Assert that we got a not-null datum, but it's probably worth expending an actual test-and-elog for the NIL-list case. Of course, someone could put a string into that column that doesn't read out as a List at all, or it does but the elements aren't Query nodes, etc etc. There's a very finite limit to how much code I'm willing to expend on such scenarios. But a test for NIL list seems reasonable, since this function previously had what looked like sane handling for that case. Anyway, I adjusted Kyotaro-san's patch a bit (including fixing the near identical code in make_viewdef) and pushed it. regards, tom lane
At Mon, 02 Nov 2020 14:49:50 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in > Ranier Vilela <ranier.vf@gmail.com> writes: > > Em seg., 2 de nov. de 2020 às 01:36, Kyotaro Horiguchi < > > horikyota.ntt@gmail.com> escreveu: > >> Doesn't seem that ev_qual and ev_action can be NULL. The same > >> function in the current converts action list to string using > >> nodeToSTring so NIL is converted into '<>', which is not NULL. > > > I think that Assert is not the right solution here. > > I think there's some confusion here: whether the ev_actions column can > contain a SQL NULL is a very different thing from whether the result of > stringToNode() on it can be a NIL list. The latter should also not > happen, but it's not enforced by low-level code in the same way that > the NOT NULL property is. So in my judgment, it's okay for us to just > Assert that we got a not-null datum, but it's probably worth expending > an actual test-and-elog for the NIL-list case. > > Of course, someone could put a string into that column that doesn't > read out as a List at all, or it does but the elements aren't Query > nodes, etc etc. There's a very finite limit to how much code I'm > willing to expend on such scenarios. But a test for NIL list seems > reasonable, since this function previously had what looked like sane > handling for that case. > > Anyway, I adjusted Kyotaro-san's patch a bit (including fixing the > near identical code in make_viewdef) and pushed it. Thanks! -- Kyotaro Horiguchi NTT Open Source Software Center