Thread: SSL-mode error reporting in libpq

In testing the fix for the SSL problem that Martin Pihlak reported, I
realized that libpq doesn't really cope very well with errors reported
by OpenSSL.  In the case at hand, SSL_write returns an SSL_ERROR_SSL
code, which pqsecure_write quite reasonably handles by putting
"SSL error: bad write retry" into conn->errorMessage.  However, it
then sets errno = ECONNRESET, which causes its caller pqSendSome()
to overwrite that potentially-useful message with an outright lie:
"server closed the connection unexpectedly".

I think what we ought to do is adjust the code so that in SSL mode,
pqsecure_write is responsible for constructing all error messages and
pqSendSome should just leave conn->errorMessage alone.

We could perhaps go a bit further and make pqsecure_write responsible
for the error message in non-SSL mode too, but it looks to me like
pqSendSome has to have a switch on the errno anyway to decide whether to
keep trying or not, so moving that responsibility would just lead to
duplicative coding.

Any objections?
        regards, tom lane

On Sun, Jul 24, 2011 at 20:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In testing the fix for the SSL problem that Martin Pihlak reported, I
> realized that libpq doesn't really cope very well with errors reported
> by OpenSSL.  In the case at hand, SSL_write returns an SSL_ERROR_SSL
> code, which pqsecure_write quite reasonably handles by putting
> "SSL error: bad write retry" into conn->errorMessage.  However, it
> then sets errno = ECONNRESET, which causes its caller pqSendSome()
> to overwrite that potentially-useful message with an outright lie:
> "server closed the connection unexpectedly".
>
> I think what we ought to do is adjust the code so that in SSL mode,
> pqsecure_write is responsible for constructing all error messages and
> pqSendSome should just leave conn->errorMessage alone.
>
> We could perhaps go a bit further and make pqsecure_write responsible
> for the error message in non-SSL mode too, but it looks to me like
> pqSendSome has to have a switch on the errno anyway to decide whether to
> keep trying or not, so moving that responsibility would just lead to
> duplicative coding.
>
> Any objections?

I haven't looked at the actual code but - does it make sense to have
them responsible for different parts and append them? If not, then no
objections ;)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Sun, Jul 24, 2011 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> We could perhaps go a bit further and make pqsecure_write responsible
> for the error message in non-SSL mode too, but it looks to me like
> pqSendSome has to have a switch on the errno anyway to decide whether to
> keep trying or not, so moving that responsibility would just lead to
> duplicative coding.
>
> Any objections?

No objection.

Some users we have have been confused by this message.  At first I
thought it was an SSL renegotiation problem, but now I realize that
the message is a lie, so anything can be the problem.  A better
message may well decrease support burden for us, and also help us find
problems...

-- 
fdr