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
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
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
Attachments:
strcmp-fixes.patchtext/x-diff; charset=us-asciiDownload+12-8
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
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