PATCH SSL_pending() checks in libpq/fe-misc.c

Hello:

I took a look at the SSL code in libpq/fe-misc.c and noticed what I
think is a small problem. A patch is included at the bottom of this
email against anoncvs TopOfTree this evening.

The SSL library buffers input data internally. Nowhere in libpq's code
is this buffer being checked via SSL_pending(), which can lead to a
condition where once in a while a socket appears to "hang" or "lag".
This is because select() won't see bytes buffered by the library. A
condition like this is most likely to occur when the library's read
buffer has been filled previously and another read is to be performed.
If the end of the backend's transmission was less than one SSL frame
payload away from the last byte returned in the previous read, this will
likely hang. Trust me that I learned of this most painfully...

I am looking deeper at how to enable non-blocking SSL sockets in libpq.
As Tom Lane states, this is primarily a matter of checking SSL error
codes, particularly for SSL_WANT_READ and SSL_WANT_WRITE, and reacting
appropriately. I'll see about that as I have more free time.

Even though I'm doing this, I tend to agree with Tom that SSH tunnels
are a really good way to make the whole SSL problem just go away.

My quick patch to perform the SSL_pending() checks:

===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-misc.c,v
retrieving revision 1.70
diff -r1.70 fe-misc.c
350a351
> * -or- if SSL is enabled and used, is it buffering bytes?
361a363,371
> /* Check for SSL library buffering read bytes */
> #ifdef USE_SSL
> if (conn->ssl && SSL_pending(conn->ssl) > 0)
> {
> /* short-circuit the select */
> return 1;
> }
> #endif
>
784a795,797
> * If SSL enabled and used and forRead, buffered bytes short-circuit the
> * call to select().
> *
801a815,823
>
> /* Check for SSL library buffering read bytes */
> #ifdef USE_SSL
> if (forRead && conn->ssl && SSL_pending(conn->ssl) > 0)
> {
> /* short-circuit the select */
> return 0;
> }
> #endif

_Of_course_ I am just fine with this patch being under a Berkeley-style
license and included in PostgreSQL.

Cheers.

--

Jack Bates
Portland, OR, USA
http://www.floatingdoghead.net

Got privacy?
My PGP key: http://www.floatingdoghead.net/pubkey.txt

Would you send over a context diff, diff -c?

---------------------------------------------------------------------------

Jack Bates wrote:
>
> Hello:
>
> I took a look at the SSL code in libpq/fe-misc.c and noticed what I
> think is a small problem. A patch is included at the bottom of this
> email against anoncvs TopOfTree this evening.
>
> The SSL library buffers input data internally. Nowhere in libpq's code
> is this buffer being checked via SSL_pending(), which can lead to a
> condition where once in a while a socket appears to "hang" or "lag".
> This is because select() won't see bytes buffered by the library. A
> condition like this is most likely to occur when the library's read
> buffer has been filled previously and another read is to be performed.
> If the end of the backend's transmission was less than one SSL frame
> payload away from the last byte returned in the previous read, this will
> likely hang. Trust me that I learned of this most painfully...
>
> I am looking deeper at how to enable non-blocking SSL sockets in libpq.
> As Tom Lane states, this is primarily a matter of checking SSL error
> codes, particularly for SSL_WANT_READ and SSL_WANT_WRITE, and reacting
> appropriately. I'll see about that as I have more free time.
>
> Even though I'm doing this, I tend to agree with Tom that SSH tunnels
> are a really good way to make the whole SSL problem just go away.
>
> My quick patch to perform the SSL_pending() checks:
>
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-misc.c,v
> retrieving revision 1.70
> diff -r1.70 fe-misc.c
> 350a351
> > * -or- if SSL is enabled and used, is it buffering bytes?
> 361a363,371
> > /* Check for SSL library buffering read bytes */
> > #ifdef USE_SSL
> > if (conn->ssl && SSL_pending(conn->ssl) > 0)
> > {
> > /* short-circuit the select */
> > return 1;
> > }
> > #endif
> >
> 784a795,797
> > * If SSL enabled and used and forRead, buffered bytes short-circuit the
> > * call to select().
> > *
> 801a815,823
> >
> > /* Check for SSL library buffering read bytes */
> > #ifdef USE_SSL
> > if (forRead && conn->ssl && SSL_pending(conn->ssl) > 0)
> > {
> > /* short-circuit the select */
> > return 0;
> > }
> > #endif
>
> _Of_course_ I am just fine with this patch being under a Berkeley-style
> license and included in PostgreSQL.
>
> Cheers.
>
> --
>
> Jack Bates
> Portland, OR, USA
> http://www.floatingdoghead.net
>
> Got privacy?
> My PGP key: http://www.floatingdoghead.net/pubkey.txt
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce:

I've attached the original source file that I modified, my modified
version, and the output from 'diff -c fe-misc.c fe-misc.c.jack'.

Lack of CVS tags makes this the best way for me to get this to you.

Let me know if you need anything else.

I am no longer pursuing a total non-blocking implementation. I haven't
found a good way to test it with the type of work that I do with
PostgreSQL. I do use blocking SSL sockets with this mod and have had no
problem whatsoever. The bug that I fixed in this patch is exceptionally
hard to reproduce reliably.

THANKS AGAIN!!!

--

Jack Bates
Portland, OR, USA
http://www.floatingdoghead.net

Got privacy?
My PGP key: http://www.floatingdoghead.net/pubkey.txt

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

jack(at)floatingdoghead(dot)net wrote:
>
> Bruce:
>
> I've attached the original source file that I modified, my modified
> version, and the output from 'diff -c fe-misc.c fe-misc.c.jack'.
>
> Lack of CVS tags makes this the best way for me to get this to you.
>
> Let me know if you need anything else.
>
> I am no longer pursuing a total non-blocking implementation. I haven't
> found a good way to test it with the type of work that I do with
> PostgreSQL. I do use blocking SSL sockets with this mod and have had no
> problem whatsoever. The bug that I fixed in this patch is exceptionally
> hard to reproduce reliably.
>
> THANKS AGAIN!!!
>
> --
>
> Jack Bates
> Portland, OR, USA
> http://www.floatingdoghead.net
>
> Got privacy?
> My PGP key: http://www.floatingdoghead.net/pubkey.txt

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026