Better sanity checking for message length words
We had a report [1]/messages/by-id/YIKCCXcEozx9iiBU@c720-r368166.fritz.box of a case where a broken client application
sent some garbage to the server, which then hung up because it
misinterpreted the garbage as a very long message length, and was
sitting waiting for data that would never arrive. There is already
a sanity check on the message type byte in postgres.c's SocketBackend
(which the trouble case accidentally got past because 'S' is a valid
type code); but the only check on the message length is that it be
at least 4.
pq_getmessage() does have the ability to enforce an upper limit on
message length, but we only use that capability for authentication
messages, and not entirely consistently even there.
Meanwhile on the client side, libpq has had simple message-length
sanity checking for ages: it disbelieves message lengths greater
than 30000 bytes unless the message type is one of a short list
of types that can be long.
So the attached proposed patch changes things to make it required
not optional for callers of pq_getmessage to provide an upper length
bound, and installs the same sort of short-vs-long message heuristic
as libpq has in the server. This is also a good opportunity for
other callers to absorb the lesson SocketBackend learned many years
ago: we should validate the message type code *before* believing
anything about the message length. All of this is just heuristic
of course, but I think it makes for a substantial reduction in the
trouble surface.
Given the small number of complaints to date, I'm hesitant to
back-patch this: if there's anybody out there with valid use for
long messages that I didn't think should be long, this might break
things for them. But I think it'd be reasonable to sneak it into
v14, since we've not started beta yet.
Thoughts?
regards, tom lane
[1]: /messages/by-id/YIKCCXcEozx9iiBU@c720-r368166.fritz.box
Attachments:
tighten-sanity-checks-on-message-lengths-1.patchtext/x-diff; charset=us-ascii; name=tighten-sanity-checks-on-message-lengths-1.patchDownload+82-19
Hi Tom,
...
Given the small number of complaints to date, I'm hesitant to
back-patch this: if there's anybody out there with valid use for
long messages that I didn't think should be long, this might break
things for them. But I think it'd be reasonable to sneak it into
v14, since we've not started beta yet.Thoughts?
I'm having slight issues applying your patch to the `master` branch.
Is it the right target?
Regarding the idea, I think extra checks are a good thing and
definitely something that can be introduced in the next major version.
If we receive a complaint during beta-testing we can revert the patch
or maybe increase the limit for small messages.
--
Best regards,
Aleksander Alekseev
Aleksander Alekseev <aleksander@timescale.com> writes:
I'm having slight issues applying your patch to the `master` branch.
Is it the right target?
[ scratches head ... ] The patch still applies perfectly cleanly
for me, using either "patch" or "git apply".
Regarding the idea, I think extra checks are a good thing and
definitely something that can be introduced in the next major version.
If we receive a complaint during beta-testing we can revert the patch
or maybe increase the limit for small messages.
Actually, I did some more testing yesterday and found that
"make check-world" passes with PQ_SMALL_MESSAGE_LIMIT values
as small as 16. That may say more about our testing than
anything else --- for example, it implies we're not using long
statement or portal names anywhere. But still, it suggests
that 30000 is between one and two orders of magnitude too large.
I'm now thinking that 10000 would be a good conservative setting,
or we could try 1000 if we want to be a bit more aggressive.
As you say, beta-testing feedback could result in further
modifications.
regards, tom lane
Hi Tom,
scratches head ... ] The patch still applies perfectly cleanly
for me, using either "patch" or "git apply".
Sorry, my bad. It was about lines separating on different platforms. The
patch is fine and passes installcheck-world on MacOS.
--
Best regards,
Aleksander Alekseev