Re: [HACKERS] Warnings around booleans
* Andres Freund (and...@anarazel.de) wrote: I'd rather see those split into separate commits. Doing polishing and active bugs in one commit imo isn't a good idea if the polishing goes beyond a line or two. Otherwise this looks ok to me. Done that way. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Warnings around booleans
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: On Friday, August 21, 2015, Tom Lane t...@sss.pgh.pa.us wrote: It is not really acceptable to leave roles hanging around after make installcheck; that would be a security hazard for the installation. Please drop them. The only ones which were left were intentionally all NOLOGIN to address that concern, which I had considered. Is there another issue with them beyond potential login that I'm missing? NOLOGIN addresses the most obvious abuse potential, but it hardly seems like the only risk. And we have never yet intended the main regression tests to serve as a testbed for pg_dumpall -g. A bugfix commit is not the place to start changing that policy. I've updated the test to drop the roles at the end. (If you want to have some testing in this area, perhaps adding roles during the pg_upgrade test would be a safer place to do it.) I'll look into this. The lack of pg_dumpall testing is pretty concerning, considering how important it is to pg_upgrade. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Warnings around booleans
Stephen Frost sfr...@snowman.net writes: On Friday, August 21, 2015, Tom Lane t...@sss.pgh.pa.us wrote: It is not really acceptable to leave roles hanging around after make installcheck; that would be a security hazard for the installation. Please drop them. The only ones which were left were intentionally all NOLOGIN to address that concern, which I had considered. Is there another issue with them beyond potential login that I'm missing? NOLOGIN addresses the most obvious abuse potential, but it hardly seems like the only risk. And we have never yet intended the main regression tests to serve as a testbed for pg_dumpall -g. A bugfix commit is not the place to start changing that policy. (If you want to have some testing in this area, perhaps adding roles during the pg_upgrade test would be a safer place to do it.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
Stephen Frost sfr...@snowman.net writes: On Friday, August 21, 2015, Piotr Stefaniak postg...@piotr-stefaniak.me wrote: If I'm not mistaken, the roles introduced in this test are never dropped, which will cause the test to fail on consequent runs. Ah, I was thinking there was a reason to not leave them around but couldn't think of it and liked the idea of testing the pg_dumpall / pg_upgrade code paths associated. Perhaps the way to address this is to add a bit of code to drop them if they exist? That would be reasonably straight-forward to do, I believe. It is not really acceptable to leave roles hanging around after make installcheck; that would be a security hazard for the installation. Please drop them. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
On Friday, August 21, 2015, Tom Lane t...@sss.pgh.pa.us wrote: Stephen Frost sfr...@snowman.net javascript:; writes: On Friday, August 21, 2015, Piotr Stefaniak postg...@piotr-stefaniak.me javascript:; wrote: If I'm not mistaken, the roles introduced in this test are never dropped, which will cause the test to fail on consequent runs. Ah, I was thinking there was a reason to not leave them around but couldn't think of it and liked the idea of testing the pg_dumpall / pg_upgrade code paths associated. Perhaps the way to address this is to add a bit of code to drop them if they exist? That would be reasonably straight-forward to do, I believe. It is not really acceptable to leave roles hanging around after make installcheck; that would be a security hazard for the installation. Please drop them. The only ones which were left were intentionally all NOLOGIN to address that concern, which I had considered. Is there another issue with them beyond potential login that I'm missing? Thanks, Stephen
Re: [HACKERS] Warnings around booleans
On Friday, August 21, 2015, Piotr Stefaniak postg...@piotr-stefaniak.me wrote: If I'm not mistaken, the roles introduced in this test are never dropped, which will cause the test to fail on consequent runs. Ah, I was thinking there was a reason to not leave them around but couldn't think of it and liked the idea of testing the pg_dumpall / pg_upgrade code paths associated. Perhaps the way to address this is to add a bit of code to drop them if they exist? That would be reasonably straight-forward to do, I believe. Will take a look at that. Thanks! Stephen
Re: [HACKERS] Warnings around booleans
On Mon, Aug 17, 2015 at 8:01 AM, Andres Freund and...@anarazel.de wrote: I'd rather see those split into separate commits. Doing polishing and active bugs in one commit imo isn't a good idea if the polishing goes beyond a line or two. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
Hi, On 2015-08-12 16:46:01 -0400, Stephen Frost wrote: From ab44660e9b8fc5b10f84ce6ba99a8340047ac8a5 Mon Sep 17 00:00:00 2001 From: Stephen Frost sfr...@snowman.net Date: Wed, 12 Aug 2015 15:50:54 -0400 Subject: [PATCH] In AlterRole, make bypassrls an int When reworking bypassrls in AlterRole to operate the same way the other attribute handling is done, I missed that the variable was incorrectly a bool rather than an int. This meant that on platforms with an unsigned char, we could end up with incorrect behavior during ALTER ROLE. Pointed out by Andres thanks to tests he did changing our bool to be the one from stdbool.h which showed this and a number of other issues. Add regression tests to test CREATE/ALTER role for the various role attributes. Arrange to leave roles behind for testing pg_dumpall, but none which have the LOGIN attribute. In passing, to avoid confusion, rename CreatePolicyStmt's 'cmd' to 'cmd_name', parse_policy_command's 'cmd' to 'polcmd', and AlterPolicy's 'cmd_datum' to 'polcmd_datum', per discussion with Noah and as a follow-up to his correction of copynodes/equalnodes handling of the CreatePolicyStmt 'cmd' field. I'd rather see those split into separate commits. Doing polishing and active bugs in one commit imo isn't a good idea if the polishing goes beyond a line or two. Otherwise this looks ok to me. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
On 08/12/2015 03:46 PM, Stephen Frost wrote: * Andres Freund (and...@anarazel.de) wrote: On 2015-08-12 08:16:09 -0400, Stephen Frost wrote: 1) gin stores/queries some bools as GinTernaryValue. Part of this is easy to fix, just adjust GinScanKeyData-entryRes to be a GinTernaryValue (it's actually is compared against MAYBE). What I find slightly worrysome is that in gin_tsquery_consistent() checkcondition_gin (returning GinTernaryValue) is passed as a callback that's expected to return bool. And the field checkcondition_gin is returning (GinChkVal-check[i]) actually is a ternary. Is there a potential corruption issue from that..? I honestly don't understand the gin code well enough to answer that. Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the impact of this? It's harmless. gin_tsquery_consistent() places a boolean array as the 'check' array, and therefore checkcondition_gin will also only return TRUEs and FALSEs, never MAYBEs. A comment to explain why that's OK would probably be in order though. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
On 2015-08-13 13:27:33 +0200, Michael Meskes wrote: Playing around a bit lead to to find that this is caused by a wrong type declaration in two places. 'isarray' is declared as bool instead of enum ARRAY_TYPE in two places. This appears to be an oversight, perhaps caused by the boolean sounding name. I vaguely remember that it used to be bool back in the day. I dug a bit back, but got tired of it around 2003 ;) Does this look right to you? If so, will you apply it? Yes and yes. Thanks for spotting, fixed in master and the back branches. Thanks! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
Playing around a bit lead to to find that this is caused by a wrong type declaration in two places. 'isarray' is declared as bool instead of enum ARRAY_TYPE in two places. This appears to be an oversight, perhaps caused by the boolean sounding name. I vaguely remember that it used to be bool back in the day. Does this look right to you? If so, will you apply it? Yes and yes. Thanks for spotting, fixed in master and the back branches. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at gmail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
On 2015-08-13 14:28:58 +0300, Heikki Linnakangas wrote:
On 08/12/2015 03:46 PM, Stephen Frost wrote:
* Andres Freund (and...@anarazel.de) wrote:
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData-entryRes to
be a GinTernaryValue (it's actually is compared against
MAYBE).
That bit looks sane to you? That appears to be an actual misdeclaration
to me.
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal-check[i]) actually is a
ternary.
Is there a potential corruption issue from that..?
I honestly don't understand the gin code well enough to answer that.
Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the
impact of this?
It's harmless. gin_tsquery_consistent() places a boolean array as the
'check' array, and therefore checkcondition_gin will also only return TRUEs
and FALSEs, never MAYBEs. A comment to explain why that's OK would probably
be in order though.
Ok. As I get warnings here it seems best to add a cast when passing the
function (i.e. (bool (*) (void *, QueryOperand *)) checkcondition_gin))
and adding a comment to checkcondition_gin() explaining that it better
only return booleans?
For reference, those are the warnings.
/home/andres/src/postgresql/src/backend/access/gin/ginlogic.c: In function
‘shimTriConsistentFn’:
/home/andres/src/postgresql/src/backend/access/gin/ginlogic.c:171:24: warning:
comparison of constant ‘2’ with boolean expression is always false
[-Wbool-compare]
if (key-entryRes[i] == GIN_MAYBE)
^
/home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c: In function
‘gin_tsquery_consistent’:
/home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c:287:13: warning:
assignment from incompatible pointer type [-Wincompatible-pointer-types]
gcv.check = check;
^
/home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c:294:8: warning:
passing argument 4 of ‘TS_execute’ from incompatible pointer type
[-Wincompatible-pointer-types]
checkcondition_gin);
^
In file included from
/home/andres/src/postgresql/src/backend/utils/adt/tsginidx.c:20:0:
/home/andres/src/postgresql/src/include/tsearch/ts_utils.h:107:13: note:
expected ‘_Bool (*)(void *, QueryOperand *) {aka _Bool (*)(void *, struct
anonymous *)}’ but argument is of type ‘GinTernaryValue (*)(void *,
QueryOperand *) {aka char (*)(void *, struct anonymous *)}’
extern bool TS_execute(QueryItem *curitem, void *checkval, bool calcnot,
^
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
On 08/13/2015 02:41 PM, Andres Freund wrote: On 2015-08-13 14:28:58 +0300, Heikki Linnakangas wrote: On 08/12/2015 03:46 PM, Stephen Frost wrote: * Andres Freund (and...@anarazel.de) wrote: On 2015-08-12 08:16:09 -0400, Stephen Frost wrote: 1) gin stores/queries some bools as GinTernaryValue. Part of this is easy to fix, just adjust GinScanKeyData-entryRes to be a GinTernaryValue (it's actually is compared against MAYBE). That bit looks sane to you? That appears to be an actual misdeclaration to me. Yeah, changing entryRes to GinTernaryValue seems good to me. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
* Andres Freund (and...@anarazel.de) wrote:
I find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...
Agreed, but not going to change it at this point.
Would love feedback on the attached. I included the variable renames
discussed previously with Noah as they're quite minor changes.
Had no trouble cherry-picking this back to 9.5.
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.
I'll look at doing this also in the rowsecurity regression suite, but I
really like having this coverage of CREATE/ALTER ROLE too, plus testing
the role dump/restore paths in pg_dumpall which I don't think were being
covered at all previously...
Thanks!
Stephen
From ab44660e9b8fc5b10f84ce6ba99a8340047ac8a5 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Wed, 12 Aug 2015 15:50:54 -0400
Subject: [PATCH] In AlterRole, make bypassrls an int
When reworking bypassrls in AlterRole to operate the same way the other
attribute handling is done, I missed that the variable was incorrectly a
bool rather than an int. This meant that on platforms with an unsigned
char, we could end up with incorrect behavior during ALTER ROLE.
Pointed out by Andres thanks to tests he did changing our bool to be the
one from stdbool.h which showed this and a number of other issues.
Add regression tests to test CREATE/ALTER role for the various role
attributes. Arrange to leave roles behind for testing pg_dumpall, but
none which have the LOGIN attribute.
In passing, to avoid confusion, rename CreatePolicyStmt's 'cmd' to
'cmd_name', parse_policy_command's 'cmd' to 'polcmd', and
AlterPolicy's 'cmd_datum' to 'polcmd_datum', per discussion with Noah
and as a follow-up to his correction of copynodes/equalnodes handling of
the CreatePolicyStmt 'cmd' field.
Back-patch to 9.5 where the AlterRole bug exists and to avoid the code
diverging due to minor changes while in alpha.
---
src/backend/commands/policy.c| 22 +--
src/backend/commands/user.c | 2 +-
src/backend/nodes/copyfuncs.c| 2 +-
src/backend/nodes/equalfuncs.c | 2 +-
src/backend/parser/gram.y| 2 +-
src/include/nodes/parsenodes.h | 2 +-
src/test/regress/expected/roleattributes.out | 236 +++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/roleattributes.sql | 85 ++
10 files changed, 339 insertions(+), 17 deletions(-)
create mode 100644 src/test/regress/expected/roleattributes.out
create mode 100644 src/test/regress/sql/roleattributes.sql
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index bcf4a8f..45326a3 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -108,25 +108,25 @@ RangeVarCallbackForPolicy(const RangeVar *rv, Oid relid, Oid oldrelid,
static char
parse_policy_command(const char *cmd_name)
{
- char cmd;
+ char polcmd;
if (!cmd_name)
elog(ERROR, unrecognized policy command);
if (strcmp(cmd_name, all) == 0)
- cmd = '*';
+ polcmd = '*';
else if (strcmp(cmd_name, select) == 0)
- cmd = ACL_SELECT_CHR;
+ polcmd = ACL_SELECT_CHR;
else if (strcmp(cmd_name, insert) == 0)
- cmd = ACL_INSERT_CHR;
+ polcmd = ACL_INSERT_CHR;
else if (strcmp(cmd_name, update) == 0)
- cmd = ACL_UPDATE_CHR;
+ polcmd = ACL_UPDATE_CHR;
else if (strcmp(cmd_name, delete) == 0)
- cmd = ACL_DELETE_CHR;
+ polcmd = ACL_DELETE_CHR;
else
elog(ERROR, unrecognized policy command);
- return cmd;
+ return polcmd;
}
/*
@@ -480,7 +480,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
int i;
/* Parse command */
- polcmd = parse_policy_command(stmt-cmd);
+ polcmd = parse_policy_command(stmt-cmd_name);
/*
* If the command is SELECT or DELETE then WITH CHECK should be NULL.
@@ -674,7 +674,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
bool replaces[Natts_pg_policy];
ObjectAddress target;
ObjectAddress myself;
- Datum cmd_datum;
+ Datum polcmd_datum;
char polcmd;
bool polcmd_isnull;
int i;
@@ -775,11 +775,11 @@ AlterPolicy(AlterPolicyStmt *stmt)
RelationGetRelationName(target_table;
/* Get policy command */
- cmd_datum = heap_getattr(policy_tuple, Anum_pg_policy_polcmd,
+ polcmd_datum = heap_getattr(policy_tuple, Anum_pg_policy_polcmd,
RelationGetDescr(pg_policy_rel),
polcmd_isnull);
Assert(!polcmd_isnull);
- polcmd =
[HACKERS] Warnings around booleans
Hi,
Forcing our bool to be stdbool.h shows up a bunch of errors and
warnings:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData-entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal-check[i]) actually is a
ternary.
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...
= rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
= convert to an int
4) _tableInfo-relreplident is a bool, should be a char
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
On 2015-08-12 10:43:51 +0200, Andres Freund wrote:
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that
way. Stephen?
Which incidentally leads to really scary results:
postgres[25069][1]=# SELECT oid, rolname, rolbypassrls FROM pg_authid WHERE
rolname = 'plain';
┌───┬─┬──┐
│ oid │ rolname │ rolbypassrls │
├───┼─┼──┤
│ 76892 │ plain │ f│
└───┴─┴──┘
(1 row)
postgres[25069][1]=# ALTER ROLE plain NOLOGIN;
ALTER ROLE
postgres[25069][1]=# SELECT oid, rolname, rolbypassrls FROM pg_authid WHERE
rolname = 'plain';
┌───┬─┬──┐
│ oid │ rolname │ rolbypassrls │
├───┼─┼──┤
│ 76892 │ plain │ t│
└───┴─┴──┘
(1 row)
The -1 isn't a valid value for a bool, which is now unsigned, and just
wraps around to true.
There are no tests catching this, which is a scary in it's own right.
So on linux/x86 this normally is only an issue when using a definition
for bool other than c.h's (which we explicitly try to cater for!). But
on other platforms the whole logic above is terminally broken: Consider
what happens when char is unsigned. That's e.g. the case on nearly all,
if not all, arm compilers.
This is reproducible today in an unmodified postgres on x86 if you add
-funsigned-char. Or, on any arm and possibly some other platforms.
Whoa, allowing the compiler to warn us is a good thing. This would have
been a fucktastically embarassing security issue.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData-entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal-check[i]) actually is a
ternary.
Is there a potential corruption issue from that..?
I honestly don't understand the gin code well enough to answer that.
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...
= rename field to bool_ (or _bool but that looks a wee close to C's
_Bool)
I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.
There's alread a _enum in the struct, so that doesn't bother my much.
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are
two
complaints:
I assume you mean AlterRole() above?
Oops.
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().
I find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
= convert to an int
Perhaps I'm missing something, but it appears to only ever be set to 0
or -1, so wouldn't it make sense to just correct it to use bool properly
instead?
Yea, you're right.
4) _tableInfo-relreplident is a bool, should be a char
This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?
With our current boolean definition this doesn't fail at all. All the
values fall into both char and unsigned char range.
WRT tests: It'd probably be a good idea to not drop the tables at the
end of replica_identity.sql.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Warnings around booleans
Andres,
* Andres Freund (and...@anarazel.de) wrote:
Forcing our bool to be stdbool.h shows up a bunch of errors and
warnings:
Wow.
1) gin stores/queries some bools as GinTernaryValue.
Part of this is easy to fix, just adjust GinScanKeyData-entryRes to
be a GinTernaryValue (it's actually is compared against MAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent()
checkcondition_gin (returning GinTernaryValue) is passed as a
callback that's expected to return bool. And the field
checkcondition_gin is returning (GinChkVal-check[i]) actually is a
ternary.
Is there a potential corruption issue from that..?
2) help_config.c has a member named 'bool' in a local struct. That
doesn't work well if bool actually is a macro. Which mean that whole
#ifndef bool patch in c.h wasn't exercised since 2003 when
help_config.c was created...
= rename field to bool_ (or _bool but that looks a wee close to C's _Bool)
I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.
3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
complaints:
I assume you mean AlterRole() above?
boolbypassrls = -1;
it's a boolean.
else if (authform-rolbypassrls || bypassrls = 0)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser to change bypassrls
attribute)));
}
...
if (bypassrls = 0)
{
new_record[Anum_pg_authid_rolbypassrls - 1] =
BoolGetDatum(bypassrls 0);
new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
}
if it's a boolean, that's always true.
It's not entirely clear to me why this was written that way. Stephen?
I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().
I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.
3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
= convert to an int
Perhaps I'm missing something, but it appears to only ever be set to 0
or -1, so wouldn't it make sense to just correct it to use bool properly
instead?
As for what's happening, it appears that we'll happily continue to go
through all of the databases even on an error condition, right? I seem
to recall seeing that happen and being a bit surprised at it, but wasn't
in a position at the time to debug it.
4) _tableInfo-relreplident is a bool, should be a char
This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?
Thanks!
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] Warnings around booleans
* Andres Freund (and...@anarazel.de) wrote: On 2015-08-12 08:16:09 -0400, Stephen Frost wrote: 1) gin stores/queries some bools as GinTernaryValue. Part of this is easy to fix, just adjust GinScanKeyData-entryRes to be a GinTernaryValue (it's actually is compared against MAYBE). What I find slightly worrysome is that in gin_tsquery_consistent() checkcondition_gin (returning GinTernaryValue) is passed as a callback that's expected to return bool. And the field checkcondition_gin is returning (GinChkVal-check[i]) actually is a ternary. Is there a potential corruption issue from that..? I honestly don't understand the gin code well enough to answer that. Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the impact of this? 2) help_config.c has a member named 'bool' in a local struct. That doesn't work well if bool actually is a macro. Which mean that whole #ifndef bool patch in c.h wasn't exercised since 2003 when help_config.c was created... = rename field to bool_ (or _bool but that looks a wee close to C's _Bool) I don't particularly like just renaming it to bool_, for my part, but certainly need to do something. There's alread a _enum in the struct, so that doesn't bother my much. Fair enough. 3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two complaints: [...] I specifically recall fixing a similar issue in this area but clearly didn't realize that it was a bool when, as I recall, I was changing it to match what we do for all of the other role attributes. In those other cases the value is an int, not a bool though. Changing it to an int here should make it line up with the rest of AlterRole(). I find that a somewhat ugly coding pattern, but since the rest of the function is written that way... Agreed. My role attributes patch had actually changed how that all was done, but once the discussion moved on to default roles instead of a bunch of additional role attributes, it felt like it would be unnecessary code churn to change that function. Perhaps we should do so anyway though. I'll do that and add regression tests for it and any others which don't get tested. My thinking on the test is to independently change each value and then poll for all role attributes set and verify that the only change made was the change expected. Do that if you like, but what I really think we should have is a test that tries to bypass rls and fails, then the user gets changes and it succeeds, and then it gets disabled and fails again. This really seems test-worthy behaviour to me. We certainly do have tests around BYPASSRLS, but not one which changes an independent role attribute and then re-tests. I'm fine with adding that test (or other tests, in general), but that won't help when another role attribute is added and someone makes a similar mistake, which I believe the test I'm thinking of will. Even if I'm unable to make that work though, someone grep'ing through our tests for places to add their new role attribute would likely catch a test that's checking all of the other ones while they might not consider what's done for just one attribute to be a generally applicable case. 4) _tableInfo-relreplident is a bool, should be a char This results in pg_dump always failing to dump out the necessary ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required, right? Is that something we can add a regression test to cover which will be exercised through the pg_upgrade path? With our current boolean definition this doesn't fail at all. All the values fall into both char and unsigned char range. WRT tests: It'd probably be a good idea to not drop the tables at the end of replica_identity.sql. Agreed. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Warnings around booleans
Hi Michael,
I'm currently investigating some of our code cleanliness issues around
booleans. Turns out that ecpg fails if C99's _Bool is used as bool
instead of typedef char bool.
Playing around a bit lead to to find that this is caused by a wrong type
declaration in two places. 'isarray' is declared as bool instead of enum
ARRAY_TYPE in two places. This appears to be an oversight, perhaps
caused by the boolean sounding name.
Does this look right to you? If so, will you apply it?
- Andres
From 8335ad3b9b96964f0f4eac7c16fb4016c6370acb Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 12 Aug 2015 21:44:49 +0200
Subject: [PATCH] ecpg: Use ARRAY_TYPE instead of a bool to store the array
type.
Using a bool isn't just somewhat ugly, but actually fails badly if bool
is mapped to C99's _Bool. This appears to be caused by an oversight
instead of a conscious decision.
---
src/interfaces/ecpg/ecpglib/execute.c | 2 +-
src/interfaces/ecpg/ecpglib/extern.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 9a56a5c..9e40f41 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -146,7 +146,7 @@ next_insert(char *text, int pos, bool questionmarks)
}
static bool
-ecpg_type_infocache_push(struct ECPGtype_information_cache ** cache, int oid, bool isarray, int lineno)
+ecpg_type_infocache_push(struct ECPGtype_information_cache ** cache, int oid, enum ARRAY_TYPE isarray, int lineno)
{
struct ECPGtype_information_cache *new_entry
= (struct ECPGtype_information_cache *) ecpg_alloc(sizeof(struct ECPGtype_information_cache), lineno);
diff --git a/src/interfaces/ecpg/ecpglib/extern.h b/src/interfaces/ecpg/ecpglib/extern.h
index 1fa21fd..263e001 100644
--- a/src/interfaces/ecpg/ecpglib/extern.h
+++ b/src/interfaces/ecpg/ecpglib/extern.h
@@ -44,7 +44,7 @@ struct ECPGtype_information_cache
{
struct ECPGtype_information_cache *next;
int oid;
- bool isarray;
+ enum ARRAY_TYPE isarray;
};
/* structure to store one statement */
--
2.3.0.149.gf3f4077.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
