Re: Password leakage avoidance

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:

* 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.

* 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?

* This could be shortened to just "return res":

+ if (!res)
+ return NULL;
+ else
+ return res;

* 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.

regards, tom lane