Обсуждение: Nonblocking libpq + openssl = ?

Hello all,

Not sure this is exactly right list, so feel free to point me to some
other as appropriate.

While working on a higher-level binding to the libpq library, I've
(likely) discovered a problem with non-blocking operation in case of
using openssl. And, it looks so striking I'd like to share my observation.

For libpq, non-blocking operation is documented as a normal supported
feature, e.g. [1]
Now, openssl transport is also documented as a normal supported feature,
e.g. [2]
I have not found anywhere in documentaion any clear warnings that
non-blocking operation and openssl transport are mutually exclusive or
might not quite work as specified in any way.

 From [1] we learn (through some intricate wording) that in order to
avoid blocking at PQgetResult() one can employ PQsetnonblocking(),
PQflush(), PQconsumeInput() and PQisBusy(), supposedly all of them
non-blocking after calling PQsetnonblocking(), although not stated
explicitely so, but otherwise it would make just no sence whatsoever, right?

Now lets have a look at e.g. PQconsumeInput():

===================
.....
/*
  * Load more data, if available. We do this no matter what state we are
  * in, since we are probably getting called because the application wants
  * to get rid of a read-select condition. Note that we will NOT block
  * waiting for more input.
  */
if (pqReadData(conn) < 0)
    return 0;

/* Parsing of the data waits till later. */
  return 1;
}
===================

It is stated that pqReadData() will NOT block. Now let's get inside:

===================
.....
/* OK, try to read some data */
retry3:
    nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd,
             conn->inBufSize - conn->inEnd);
.....
/*
  * Still not sure that it's EOF, because some data could have just
  * arrived.
  */
retry4:
    nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd,
            conn->inBufSize - conn->inEnd);
....
====================

Now in case of SSL, this pqsecure_read() is just a wrapper around
pgtls_read(), so lets look further:

====================
pgtls_read(PGconn *conn, void *ptr, size_t len)
{
.....
rloop:
    SOCK_ERRNO_SET(0);
    n = SSL_read(conn->ssl, ptr, len);
    err = SSL_get_error(conn->ssl, n);
    switch (err)
    {
......
            break;
    case SSL_ERROR_WANT_WRITE:
    /* Returning 0 here would cause caller to wait for read-ready,
     * which is not correct since what SSL wants is wait for
     * write-ready.  The former could get us stuck in an infinite
     * wait, so don't risk it; busy-loop instead. */
    goto rloop;
======================

So going PQconsumeInput()->pqReadData()->pqsecure_read()->pgtls_read()
in a supposedly non-blocking operation we finally come to a tight
busy-loop waiting for SSL_ERROR_WANT_WRITE to go down! How could such
thing ever be,

- with no even sleep(1),
- no timeout,
- no diagnostics of any sort,
- a comment implying that getting stuck in a (potentially) infinite
sleepless loop deep inside a library is OK.

And looking more into this pgtls_read() function it seems it just has
inadequate interface. So that it has really no way to reliably indicate
some important details to its caller, namely the need to wait for
write-readyness. It's like if ssl support was a quick-n-dirty hack
rather than a consistently integrated feature. Or do I read it all wrong?
Any thoughts?

[1] https://www.postgresql.org/docs/9.5/static/libpq-async.html
[2] https://www.postgresql.org/docs/9.5/static/libpq-ssl.html

Thank you,
Regards,

Nikolai

Hi,

> So going PQconsumeInput()->pqReadData()->pqsecure_read()->pgtls_read() in a
> supposedly non-blocking operation we finally come to a tight busy-loop
> waiting for SSL_ERROR_WANT_WRITE to go down! How could such thing ever be,
>
> - with no even sleep(1),
> - no timeout,
> - no diagnostics of any sort,
> - a comment implying that getting stuck in a (potentially) infinite
> sleepless loop deep inside a library is OK.

> And looking more into this pgtls_read() function it seems it just has
> inadequate interface. So that it has really no way to reliably indicate some
> important details to its caller, namely the need to wait for
> write-readyness. It's like if ssl support was a quick-n-dirty hack rather
> than a consistently integrated feature. Or do I read it all wrong?
> Any thoughts?

Well, it's not pretty. I quite dislike this bit, and I've complained
about it before.  But it is noteworthy that it's nearly impossible to
hit these days, due to ssl-renegotiation support having been ripped out.
That's what could trigger openssl to require writes upon reads.

Greetings,

Andres Freund

17.09.2016 2:05, Andres Freund:
[...]
> Well, it's not pretty. I quite dislike this bit, and I've complained
> about it before.  But it is noteworthy that it's nearly impossible to
> hit these days, due to ssl-renegotiation support having been ripped out.
> That's what could trigger openssl to require writes upon reads.

Looks like it _usually_ happens so that such interdependent reads and
writes are unnecessary in the absence of renegotiations. But still [1]
instructs to always check for both SSL_ERROR_WANT_READ and
SSL_ERROR_WANT_WRITE in all cases. Supposedly it is for a reason. The
way it is implemented in fe-secure-openssl.c looks just somewhat unfinished.
I'm wondering is there really something that prevents doing it properly?

[1] https://www.openssl.org/docs/manmaster/ssl/SSL_get_error.html

Thank you,
Regards,
Nikolai

>
> Greetings,
>
> Andres Freund
>
>

On 2016-09-17 03:12:53 +0300, Nikolai Zhubr wrote:
> 17.09.2016 2:05, Andres Freund:
> [...]
> > Well, it's not pretty. I quite dislike this bit, and I've complained
> > about it before.  But it is noteworthy that it's nearly impossible to
> > hit these days, due to ssl-renegotiation support having been ripped out.
> > That's what could trigger openssl to require writes upon reads.
>
> Looks like it _usually_ happens so that such interdependent reads and writes
> are unnecessary in the absence of renegotiations. But still [1] instructs to
> always check for both SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE in all
> cases. Supposedly it is for a reason. The way it is implemented in
> fe-secure-openssl.c looks just somewhat unfinished.
> I'm wondering is there really something that prevents doing it properly?

The relevant user-level API of libpq (PQisBusy) doesn't have a way to
return "waiting for write". So we'd have to break API compatibility.

Greetings,

Andres Freund

17.09.2016 3:27, Andres Freund:
[...]
>> Looks like it _usually_ happens so that such interdependent reads and writes
>> are unnecessary in the absence of renegotiations. But still [1] instructs to
>> always check for both SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE in all
>> cases. Supposedly it is for a reason. The way it is implemented in
>> fe-secure-openssl.c looks just somewhat unfinished.
>> I'm wondering is there really something that prevents doing it properly?
>
> The relevant user-level API of libpq (PQisBusy) doesn't have a way to
> return "waiting for write". So we'd have to break API compatibility.

Ah, I see. But then, this is a very common sort of problem (Existing API
spec getting inadequate for some new features added later, maintaining
complete interface compatability getting impossible.)

In this specific case, I'd say a reasonable approach would be to
urgently introduce some new PQisBusyParams() returning the flag in
question, and subsequently deprecating the historical PQisBusy(). Maybe
something else would be necessary. Meanwhile, it would seem logical to
move this busy-loop to PQisBusy() so it would become more evident
PQisBusy() is flawed.

Besides, even with no changes to API, one good thing can be done still.
If SSL_ERROR_WANT_WRITE is so unlikely to ever happen in pgtls_read(),
why not just throw a (descriptive enough) error and get out immediately?
And see if someone compains about dropped connections because of this?

And while we are at it, it would be nice to have something like
pqWaitTimed() included in the API, so as to be able to (mostly) avoid
messing with the underlying OS handles/sockets outside of libpq (and
keeping user code more generic therefore). Is there a reason for not
providing this?


Thank you,
Regards,
Nikolai

>
> Greetings,
>
> Andres Freund
>
>

17.09.2016 11:15, I wrote:
[...]
>> The relevant user-level API of libpq (PQisBusy) doesn't have a way to
>> return "waiting for write". So we'd have to break API compatibility.
>
> Ah, I see. But then, this is a very common sort of problem (Existing API
> spec getting inadequate for some new features added later, maintaining
> complete interface compatability getting impossible.)
>
> In this specific case, I'd say a reasonable approach would be to
> urgently introduce some new PQisBusyParams() returning the flag in
> question, and subsequently deprecating the historical PQisBusy(). Maybe
> something else would be necessary. Meanwhile, it would seem logical to
> move this busy-loop to PQisBusy() so it would become more evident
> PQisBusy() is flawed.

I'm now doing a respective cleanup I'll use for myself in my project.

Also, I'd be happy to share my changes to libpq so that they could
eventually be used upstream, so I'll try to post it to pgsql-hackers
when ready.


Thank you,
Regards,
Nikolai

17.09.2016 3:27, Andres Freund:
[...]
> The relevant user-level API of libpq (PQisBusy) doesn't have a way to
> return "waiting for write". So we'd have to break API compatibility.

I think this is actually not entirely correct.
PQisBusy is fine, I do not see a need to touch it at all.
Rather, the return value of PQConsumeInput could be extended (in fact
has just been extended by me here) as follows:

Old spec:
0 == error
1 == no error

New spec:
0 == error
1 == no error
2 == no error but can't proceed until pending write finished.

Agreed, this is still an incompatible API change, but not excessively
destructive and it should be fairly easy to make adjustments for such
change (unless the usage has already been hopelessly broken).


Thank you,
Regards,
Nikolai

>
> Greetings,
>
> Andres Freund
>
>