Michael Paquier <michael.paquier@gmail.com> writes:
> OK, I think that we had better address this bug for this CF. I am
> attaching an updated patch following Marko's suggestion, and marking
> this patch as ready for committer. I have noticed that the fix was
> actually not complete: ConnectDatabase needs a similar fix, this is a
> code path when a connection string like "dbname=db user=foo" is used.
> And this was failing as well when the password is passed directly in
> the connection string for multiple jobs.
As written, this would leak password strings, and it even seems possible
that it would leave savedPassword pointing at dangling memory, since the
free(password) inside the loop might free what savedPassword is pointing
at, and then in principle we might fail to overwrite savedPassword
afterwards. This probably can't happen in practice because it'd require
successive connection attempts to come to different conclusions about
PQconnectionNeedsPassword/PQconnectionUsedPassword. But it seems pretty
fragile in the face of future changes to this code. I modified it further
so that "password" and "savedPassword" never share storage, and pushed it.
A larger concern is that I suspect we need to abandon this whole approach
of passing a different representation of the connection parameters than
we were given the first time through. If dbname is a connection URI
(or keyword=value connection string), it might well contain more
information than just host + port + username + password. We're losing
any such details during the workers' reconnections. But that looks like
it would be a rather wide-ranging rewrite, so I just committed what we
had for now; at least we fixed the reported bug symptom.
regards, tom lane
