Suspicious strcmp() in src/backend/parser/parse_expr.c

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Suspicious strcmp() in src/backend/parser/parse_expr.c

Rikard Falkeborn
In src/backend/parser/parse_expr.c the following snippet of code is found (lines 3238-3242, rev 765525c8c2c6e55abe):

    if (strcmp(*nodename, "+") == 0 ||
            strcmp(*nodename, "-")) // <-- notice the lack of comparisson here
            group = 0;
    else
            group = PREC_GROUP_PREFIX_OP;

Should the second part of the || be strcmp(*nodename, "-") == 0?
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Andrew Gierth
>>>>> "Rikard" == Rikard Falkeborn <[hidden email]> writes:

 Rikard> In src/backend/parser/parse_expr.c the following snippet of code is found
 Rikard> (lines 3238-3242, rev 765525c8c2c6e55abe):

 Rikard>     if (strcmp(*nodename, "+") == 0 ||
 Rikard>             strcmp(*nodename, "-")) // <-- notice the lack of comparisson
 Rikard> here
 Rikard>             group = 0;
 Rikard>     else
 Rikard>             group = PREC_GROUP_PREFIX_OP;

 Rikard> Should the second part of the || be strcmp(*nodename, "-") ==
 Rikard> 0?

Yes it should.

The effect of this bug is to produce a false operator precedence warning
when those are enabled, like so:

postgres=# set operator_precedence_warning = on;
SET
postgres=# select -random() is null;
WARNING:  operator precedence change: IS is now lower precedence than -

when in fact "-random() is null" always did parse as "(-random()) is null"
making the warning spurious.

--
Andrew (irc:RhodiumToad)


Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Tom Lane-2
Andrew Gierth <[hidden email]> writes:
> "Rikard" == Rikard Falkeborn <[hidden email]> writes:
>  Rikard>     if (strcmp(*nodename, "+") == 0 ||
>  Rikard>             strcmp(*nodename, "-")) // <-- notice the lack of comparisson here
>  Rikard> Should the second part of the || be strcmp(*nodename, "-") == 0?

> Yes it should.

Indeed.  Considering how much I hate using strcmp's result as a boolean,
you'd think I'd have got that right.  Thanks for noticing!

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Michael Paquier-2
On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
> Indeed.  Considering how much I hate using strcmp's result as a boolean,
> you'd think I'd have got that right.  Thanks for noticing!

Just a note about those strcmp() calls using a boolean as return
result in the tree:
src/backend/commands/lockcmds.c: (!strcmp(rte->eref->aliasname, "old")
|| !strcmp(rte->eref->aliasname, "new")))
src/test/modules/test_rls_hooks/test_rls_hooks.c:   if
(strcmp(RelationGetRelationName(relation), "rls_test_permissive")
src/test/modules/test_rls_hooks/test_rls_hooks.c:       &&
strcmp(RelationGetRelationName(relation), "rls_test_both"))
src/test/modules/test_rls_hooks/test_rls_hooks.c:   if
(strcmp(RelationGetRelationName(relation), "rls_test_restrictive")
src/test/modules/test_rls_hooks/test_rls_hooks.c:       &&
strcmp(RelationGetRelationName(relation), "rls_test_both"))

Would it be worth changing these as well?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Thomas Munro-5
On Thu, Apr 11, 2019 at 2:20 PM Michael Paquier <[hidden email]> wrote:
> On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
> > Indeed.  Considering how much I hate using strcmp's result as a boolean,
> > you'd think I'd have got that right.  Thanks for noticing!
>
> Just a note about those strcmp() calls using a boolean as return
> result in the tree:
> src/backend/commands/lockcmds.c: (!strcmp(rte->eref->aliasname, "old")

Don't look at contrib/spi/refint.c if you value your sanity.

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

David Rowley-3
On Thu, 11 Apr 2019 at 15:27, Thomas Munro <[hidden email]> wrote:

>
> On Thu, Apr 11, 2019 at 2:20 PM Michael Paquier <[hidden email]> wrote:
> > On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
> > > Indeed.  Considering how much I hate using strcmp's result as a boolean,
> > > you'd think I'd have got that right.  Thanks for noticing!
> >
> > Just a note about those strcmp() calls using a boolean as return
> > result in the tree:
> > src/backend/commands/lockcmds.c: (!strcmp(rte->eref->aliasname, "old")
>
> Don't look at contrib/spi/refint.c if you value your sanity.

hmm, yeah. Take a non-bool expression, make it into a bool expression,
then compare that to 0 to make it look like a non-bool expression.
Weird.

If we're fixing some, we may as well do them all.

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Michael Paquier-2
In reply to this post by Thomas Munro-5
On Thu, Apr 11, 2019 at 03:26:31PM +1200, Thomas Munro wrote:
> Don't look at contrib/spi/refint.c if you value your sanity.

Perhaps I don't value it much then.  At least it compares to 0 after
compiling a set of bool results.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Tom Lane-2
In reply to this post by Michael Paquier-2
Michael Paquier <[hidden email]> writes:
> On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
>> Indeed.  Considering how much I hate using strcmp's result as a boolean,
>> you'd think I'd have got that right.  Thanks for noticing!

> Would it be worth changing these as well?

I'd be +1 for that, just on the grounds of having consistent coding
style.  But I'm not sufficiently excited about it to do the work
myself ...

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Michael Paquier-2
On Thu, Apr 11, 2019 at 12:55:49AM -0400, Tom Lane wrote:
> I'd be +1 for that, just on the grounds of having consistent coding
> style.  But I'm not sufficiently excited about it to do the work
> myself ...

Well, here you go as per the attached as we are on it.  I am not
noticing any other spots.
--
Michael

strcmp-fixes.patch (2K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

David Rowley-3
On Thu, 11 Apr 2019 at 18:51, Michael Paquier <[hidden email]> wrote:
>
> On Thu, Apr 11, 2019 at 12:55:49AM -0400, Tom Lane wrote:
> > I'd be +1 for that, just on the grounds of having consistent coding
> > style.  But I'm not sufficiently excited about it to do the work
> > myself ...
>
> Well, here you go as per the attached as we are on it.  I am not
> noticing any other spots.

Patch looks fine to me.  I also made a quick pass and noticed a few
more things out of place.

formatting.c in NUM_prepare_locale()

else if (strcmp(Np->decimal, ",") !=0)


spell.c in NISortDictionary()

if (i == 0
|| strcmp(Conf->Spell[i]->p.flag, Conf->Spell[i - 1]->p.flag))

if (i == 0
|| strcmp(Conf->Spell[i]->p.flag, Conf->AffixData[curaffix]))

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Tom Lane-2
David Rowley <[hidden email]> writes:
> formatting.c in NUM_prepare_locale()

> else if (strcmp(Np->decimal, ",") !=0)

I'll bet a nickel that that's pgindent's fault.  It probably thinks
that "decimal" is a typedef (greps typedefs.list ... yes), and that
tends to result in funny space-omissions later in the line.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Michael Paquier-2
On Thu, Apr 11, 2019 at 10:12:06AM -0400, Tom Lane wrote:
> I'll bet a nickel that that's pgindent's fault.  It probably thinks
> that "decimal" is a typedef (greps typedefs.list ... yes), and that
> tends to result in funny space-omissions later in the line.

Yes, I can see as well that pgindent complains here.  As that would
result in more noise on follow-up pgindent runs, I am not changing
this one.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

Michael Paquier-2
In reply to this post by David Rowley-3
On Fri, Apr 12, 2019 at 02:06:11AM +1200, David Rowley wrote:
> spell.c in NISortDictionary()
>
> if (i == 0
> || strcmp(Conf->Spell[i]->p.flag, Conf->Spell[i - 1]->p.flag))
>
> if (i == 0
> || strcmp(Conf->Spell[i]->p.flag, Conf->AffixData[curaffix]))

Good catch on these two.  I have included these and fixed all the
spots found in d527fda.
--
Michael

signature.asc (849 bytes) Download Attachment