Thread: Suspicious strcmp() in src/backend/parser/parse_expr.c
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?
>>>>> "Rikard" == Rikard Falkeborn <rikard.falkeborn@gmail.com> 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)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Rikard" == Rikard Falkeborn <rikard.falkeborn@gmail.com> 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
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
Attachment
On Thu, Apr 11, 2019 at 2:20 PM Michael Paquier <michael@paquier.xyz> 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
On Thu, 11 Apr 2019 at 15:27, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Thu, Apr 11, 2019 at 2:20 PM Michael Paquier <michael@paquier.xyz> 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
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
Attachment
Michael Paquier <michael@paquier.xyz> 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
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
Attachment
On Thu, 11 Apr 2019 at 18:51, Michael Paquier <michael@paquier.xyz> 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
David Rowley <david.rowley@2ndquadrant.com> 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
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
Attachment
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