Re: BUG #12799: libpq - SSL pqsecure_read() doesn't clean openssl error queue before reading

On 02/24/2015 05:09 AM, william(dot)welter(at)4linux(dot)com(dot)br wrote:
> According to OpenSSL manual
> (https://www.openssl.org/docs/ssl/SSL_get_error.html#DESCRIPTION) "The
> current thread's error queue must be empty before the TLS/SSL I/O operation
> is attempted, or SSL_get_error() will not work reliably"
>
> But libpq in pgsecure_read()/pqsecure_write() on branch REL9_4_STABLE or in
> pgtls_read()/pgtls_write() on branch MASTER there no calls to
> ERR_clear_error() to clear error queue
> (https://www.openssl.org/docs/crypto/ERR_clear_error.html) and avoid
> unpredictable conditions.

<pedantic>
The OpenSSL manual doesn't directly require you to call
ERR_clear_error() before every SSL_* call. It just requires that you
ensure that the error queue is empty. Libpq ensures that by always
clearing the queue *after* an error happens, in SSLerrmessage().
</pedantic>

I'm actually not 100% sure we clear the error queue after every error.
Looking at the OpenSSL sources on SSL_get_error(), it seems possible
that it returns SSL_ERROR_SYSCALL, with an error in the error queue. The
libpq code does not call SSLerrmessag() when SSL_get_error() returns
SSL_ERROR_SYSCALL, so that would leave the error dangling in the queue.
Also, if there are multiple errors in the queue, we only return and
remove from the queue the first one.

So should we clear the error queue before each call, just to be sure?
The risk is that the application has performed some other OpenSSL
operations, outside libpq, but not checked the error queue yet, and when
we clear the queue, the original error is gone and isn't reported. An
application probably shouldn't do that, you should check the error after
each SSL call, and not leave the error dangling in the error queue as a
landmine for the next SSL call. OTOH, if the application is built so
that the error queue is cleared before each SSL call, not after, that's
incompatible with the way libpq currently works. And that's certainly a
much more common practice than the hypothetical application that would
miss the error if it's cleared unexpectedly.

Overall, I agree we should clear the error queue before each SSL call.
It makes the application as whole more robust.

> The solution is simple, see following patches:

Yeah. Should also do the same in the backend, and also before
SSL_connect/SSL_accept() calls. I.e. any time SSL_get_error is used.

- Heikki