Re: 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:
>
> 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