From: | Joe Conway <mail(at)joeconway(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgreSQL(dot)org>, Dave Cramer <davecramer(at)postgres(dot)rocks> |
Subject: | Re: Password leakage avoidance |
Date: | 2024-01-07 18:51:50 |
Message-ID: | 6e670f8b-d09c-4331-a376-fef3e045f5d2@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 1/6/24 15:10, Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> The only code specific comments were Tom's above, which have been
>> addressed. If there are no serious objections I plan to commit this
>> relatively soon.
>
> I had not actually read this patchset before, but now I have, and
> I have a few minor suggestions:
Many thanks!
> * The API comment for PQchangePassword should specify that encryption
> is done according to the server's password_encryption setting, and
> probably the SGML docs should too. You shouldn't have to read the
> code to discover that.
Check
> * I don't especially care for the useless initializations of
> encrypted_password, fmtpw, and fmtuser. In all those cases the
> initial NULL is immediately replaced by a valid value. Probably
> the compiler will figure out that the initializations are useless,
> but human readers have to do so as well. Moreover, I think this
> style is more bug-prone not less so, because if you ever change
> the logic in a way that causes some code paths to fail to set
> the variables, you won't get use-of-possibly-uninitialized-value
> warnings from the compiler.
>
> * Perhaps move the declaration of "buf" to the inner block where
> it's actually used?
Makes sense -- fixed
> * This could be shortened to just "return res":
>
> + if (!res)
> + return NULL;
> + else
> + return res;
Heh, apparently I needed more coffee at this point :-)
> * I'd make the SGML documentation a bit more explicit about the
> return value, say
>
> + Returns a <structname>PGresult</structname> pointer representing
> + the result of the <literal>ALTER USER</literal> command, or
> + a null pointer if the routine failed before issuing any command.
Fixed.
I also ran pgindent. I was kind of surprised/unhappy when it made me
change this (which aligned the two var names):
8<------------
<tab><tab><tab><tab>PQExpBufferData<tab>buf;
<tab><tab><tab><tab>PGresult<tab><sp><sp><sp>*res;
8<------------
to this (which leaves the var names unaligned):
8<------------
<tab><tab><tab><tab>PQExpBufferData<sp>buf;
<tab><tab><tab><tab>PGresult<sp><sp><sp>*res;
8<------------
Anyway, the resulting adjustments attached.
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachment | Content-Type | Size |
---|---|---|
v2-0001-change-pw-libpq.patch | text/x-patch | 3.7 KB |
v2-0002-change-pw-psql.patch | text/x-patch | 1.1 KB |
v2-0003-change-pw-doc.patch | text/x-patch | 2.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Cheshev | 2024-01-07 22:55:01 | Re: Multidimensional Histograms |
Previous Message | Tomas Vondra | 2024-01-07 18:36:17 | Re: Multidimensional Histograms |