Thread: Re: libpq: Process buffered SSL read bytes to support records >8kB on async API

On Sun, Sep 8, 2024 at 1:08 PM Lars Kanis <lars@greiz-reinsdorf.de> wrote:
> I'm the maintainer of ruby-pg the ruby interface to the PostgreSQL
> database. This binding uses the asynchronous API of libpq by default to
> facilitate the ruby IO wait and scheduling mechanisms.
>
> This works well with the vanilla postgresql server, but it leads to
> starvation with other types of servers using the postgresql wire
> protocol 3. This is because the current functioning of the libpq async
> interface depends on a maximum size of SSL records of 8kB.

Thanks for the report! I wanted evidence that this wasn't a
ruby-pg-specific problem, so I set up a test case with
Python/psycopg2.

I was able to reproduce a hang when all of the following were true:
- psycopg2's async mode was enabled
- the client performs a PQconsumeInput/PQisBusy loop, waiting on
socket read events when the connection is busy (I used
psycopg2.extras.wait_select() for this)
- the server splits a large message over many large TLS records
- the server packs the final ReadyForQuery message into the same
record as the split message's final fragment

Gory details of the packet sizes, if it's helpful:
- max TLS record size is 12k, because it made the math easier
- server sends DataRow of 32006 bytes, followed by DataRow of 806
bytes, followed by CommandComplete/ReadyForQuery
- so there are three TLS records on the wire containing
  1) DataRow 1 fragment 1 (12k bytes)
  2) DataRow 1 fragment 2 (12k bytes)
  3) DataRow 1 fragment 3 (7430 bytes) + DataRow 2 (806 bytes)
       + CommandComplete + ReadyForQuery

> To fix this issue the attached patch calls pqReadData() repeatedly in
> PQconsumeInput() until there is no buffered SSL data left to be read.
> Another solution could be to process buffered SSL read bytes in
> PQisBusy() instead of PQconsumeInput() .

I agree that PQconsumeInput() needs to ensure that the transport
buffers are all drained. But I'm not sure this is a complete solution;
doesn't GSS have the same problem? And are there any other sites that
need to make the same guarantee before returning?

I need to switch away from this for a bit. Would you mind adding this
to the next Commitfest as a placeholder?

    https://commitfest.postgresql.org/50/

Thanks,
--Jacob

Thank you Jacob for verifying this issue!

> Gory details of the packet sizes, if it's helpful:
> - max TLS record size is 12k, because it made the math easier
> - server sends DataRow of 32006 bytes, followed by DataRow of 806
> bytes, followed by CommandComplete/ReadyForQuery
> - so there are three TLS records on the wire containing
>    1) DataRow 1 fragment 1 (12k bytes)
>    2) DataRow 1 fragment 2 (12k bytes)
>    3) DataRow 1 fragment 3 (7430 bytes) + DataRow 2 (806 bytes)
>         + CommandComplete + ReadyForQuery

How did you verify the issue on the server side - with YugabyteDB or 
with a modified Postgres server? I'd like to verify the GSSAPI part and 
I'm familiar with the Postgres server only.


> I agree that PQconsumeInput() needs to ensure that the transport
> buffers are all drained. But I'm not sure this is a complete solution;
> doesn't GSS have the same problem? And are there any other sites that
> need to make the same guarantee before returning?

Which other sites do you mean? The synchronous transfer already works, 
since the select() is short-circuit in case of pending bytes: [1]


> I need to switch away from this for a bit. Would you mind adding this
> to the next Commitfest as a placeholder?

No problem; registered: https://commitfest.postgresql.org/50/5251/

--

Regards, Lars

[1] 

https://github.com/postgres/postgres/blob/77761ee5dddc0518235a51c533893e81e5f375b9/src/interfaces/libpq/fe-misc.c#L1070

On Wed, Sep 11, 2024 at 12:08 PM Lars Kanis <lars@greiz-reinsdorf.de> wrote:
> How did you verify the issue on the server side - with YugabyteDB or
> with a modified Postgres server? I'd like to verify the GSSAPI part and
> I'm familiar with the Postgres server only.

Neither, unfortunately -- I have a protocol testbed that I use for
this kind of stuff. I'm happy to share once I get it cleaned up, but
it's unlikely to help you in this case since I haven't implemented
gssenc support. Patching the Postgres server itself seems like a good
way to go.

> > And are there any other sites that
> > need to make the same guarantee before returning?
>
> Which other sites do you mean?

I'm mostly worried that other parts of libpq might assume that a
single call to pqReadData will drain the buffers. If not, great! --
but I haven't had time to check all the call sites.

> > I need to switch away from this for a bit. Would you mind adding this
> > to the next Commitfest as a placeholder?
>
> No problem; registered: https://commitfest.postgresql.org/50/5251/

Thank you!

--Jacob

On Thu, 12 Sept 2024 at 04:30, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Wed, Sep 11, 2024 at 12:08 PM Lars Kanis <lars@greiz-reinsdorf.de> wrote:
> > How did you verify the issue on the server side - with YugabyteDB or
> > with a modified Postgres server? I'd like to verify the GSSAPI part and
> > I'm familiar with the Postgres server only.
>
> Neither, unfortunately -- I have a protocol testbed that I use for
> this kind of stuff. I'm happy to share once I get it cleaned up, but
> it's unlikely to help you in this case since I haven't implemented
> gssenc support. Patching the Postgres server itself seems like a good
> way to go.
>
> > > And are there any other sites that
> > > need to make the same guarantee before returning?
> >
> > Which other sites do you mean?
>
> I'm mostly worried that other parts of libpq might assume that a
> single call to pqReadData will drain the buffers. If not, great! --
> but I haven't had time to check all the call sites.

@Jacob, could you find some time to wrap this up? This will help us
assess whether it can be refined into a committable state soon.

Regards,
Vignesh

On Sun, Mar 16, 2025 at 6:14 AM vignesh C <vignesh21@gmail.com> wrote:
> @Jacob, could you find some time to wrap this up? This will help us
> assess whether it can be refined into a committable state soon.

This is a bug report that likely requires backpatching; feature freeze
should not be an issue here.

Thanks,
--Jacob

On Tue, Sep 10, 2024 at 11:49 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I need to switch away from this for a bit.

"a bit"

In an effort to get this unstuck (and ideally solved during this
commitfest) here are my most recent thoughts:

> I agree that PQconsumeInput() needs to ensure that the transport
> buffers are all drained. But I'm not sure this is a complete solution;
> doesn't GSS have the same problem? And are there any other sites that
> need to make the same guarantee before returning?

So, Postgres internals cheat: calls to
pqWait/pqWaitTimed/pqSocketCheck() all reach down into the SSL buffer
to see if there are any pending bytes before polling the socket. I do
not yet understand why this protection is not extended to
GSS-encrypted connections.

In any case, our clients can't make use of that protection either, so
it's unfortunate that we're relying on it ourselves, because it makes
reasoning about a fix more difficult. I'm not about to propose
removing that case in a backport, but as long as it's there covering
up the problem, it's hard to say whether we've truly fixed this bug.

I think the hang could happen in any situation where a third-party
client calls into a function which calls pqReadData(), and then
immediately polls the socket for a read-ready condition. So: what are
the APIs that could be called that way?

Clearly PQconsumeInput() is one. PQconnectPoll() seems like it should
be exposed as well. postgres_fdw's use of PQgetResult() might qualify
(or if it doesn't, I don't understand why not -- there is at least one
path in PQgetResult() where the buffer is not fully consumed). And
nonblocking mode seems like it might sprinkle this hazard into more
places, as pqSendSome() will then start reading data and returning
before a pqWait().

So, to fix this once and for all, do we have to make sure that
pqReadData() always fully drains the SSL/GSS transport buffer instead
of eagerly returning?

--Jacob

On Tue, Jul 1, 2025 at 1:42 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I do
> not yet understand why this protection is not extended to
> GSS-encrypted connections.

After repurposing some of my test code for d98cefe11, I'm able to
reproduce the hang with gssencmode when the server uses a
smaller-than-standard (12k) send buffer. Same reproduction case as the
original (32006-byte DataRow, 806-byte DataRow, CommandComplete,
ReadyForQuery).

So a complete patch for this has to consider GSS as well.

> So, to fix this once and for all, do we have to make sure that
> pqReadData() always fully drains the SSL/GSS transport buffer instead
> of eagerly returning?

I'll work on proving that code paths other than PQconsumeInput() are
affected. If they are, I'll start a patch for pqReadData().

--Jacob