Thread: 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
againstMAYBE).
What I find slightly worrysome is that in gin_tsquery_consistent() checkcondition_gin (returning GinTernaryValue) is
passedas 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.
Whichmean 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:
bool bypassrls = -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
On 2015-08-12 10:43:51 +0200, Andres Freund wrote:
> 3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
> complaints:
>
> bool bypassrls = -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
Andres,
* Andres Freund (andres@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?
> bool bypassrls = -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
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.
> > bool bypassrls = -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.
* Andres Freund (andres@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
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
Attachment
* Andres Freund (andres@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
Attachment
> 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
On 08/12/2015 03:46 PM, Stephen Frost wrote: > * Andres Freund (andres@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
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!
On 2015-08-13 14:28:58 +0300, Heikki Linnakangas wrote:
> On 08/12/2015 03:46 PM, Stephen Frost wrote:
> >* Andres Freund (andres@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
expressionis 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
incompatiblepointer 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
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 (andres@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
Hi, On 2015-08-12 16:46:01 -0400, Stephen Frost wrote: > From ab44660e9b8fc5b10f84ce6ba99a8340047ac8a5 Mon Sep 17 00:00:00 2001 > From: Stephen Frost <sfrost@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
On Mon, Aug 17, 2015 at 8:01 AM, Andres Freund <andres@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
* Andres Freund (andres@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
If I'm not mistaken, the roles introduced in this test are never dropped, which will cause the test to fail on consequent runs.
On Friday, August 21, 2015, Piotr Stefaniak <postgres@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
Stephen Frost <sfrost@snowman.net> writes:
> On Friday, August 21, 2015, Piotr Stefaniak <postgres@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
On Friday, August 21, 2015, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Stephen Frost <sfrost@snowman.net> writes:
> On Friday, August 21, 2015, Piotr Stefaniak <postgres@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.
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
Stephen Frost <sfrost@snowman.net> writes:
> On Friday, August 21, 2015, Tom Lane <tgl@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
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > On Friday, August 21, 2015, Tom Lane <tgl@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