On 11/8/22 00:40, Peter Eisentraut wrote:
> On 02.11.22 20:02, Jacob Champion wrote:
>> This new code path doesn't go through the message length checks that are
>> done for the 'R' and 'E' cases, and pqGetNegotiateProtocolVersion3()
>> doesn't take the message length to know where to stop anyway, so a
>> misbehaving server can chew up client resources.
>
> Fixed in new patch.
pqGetNegotiateProtocolVersion3() is still ignoring the message length,
though; it won't necessarily stop at the message boundary.
> We could add negotiation in the future, but then we'd have to first have
> a concrete case of something to negotiate about. For example, if we
> added an optional performance feature into the protocol, then one could
> negotiate by falling back to not using that. But for the kinds of
> features I'm thinking about right now (column encryption), you can't
> proceed if the feature is not supported. So I think this would need to
> be considered case by case.
I guess I'm wondering about the definition of "minor" version if the
client treats an increment as incompatible by default. But that's a
discussion for the future, and this patch is just improving the existing
behavior, so I'll pipe down and watch.
>> I think the documentation on NegotiateProtocolVersion (not introduced in
>> this patch) is misleading/wrong; it says that the version number sent
>> back is the "newest minor protocol version supported by the server for
>> the major protocol version requested by the client" which doesn't seem
>> to match the actual usage seen here.
>
> I don't follow. If libpq sends a protocol version of 3.1, then the
> server responds by saying it supports only 3.0. What are you seeing?
I see what you've described on my end, too. The sentence I quoted seemed
to imply that the server should respond with only the minor version (the
least significant 16 bits). I think it should probably just say "newest
protocol version" in the docs.
Thanks,
--Jacob