On Sun, 7 Apr 2024 at 22:05, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Sun, 7 Apr 2024 at 03:39, Andres Freund <andres@anarazel.de> wrote:
> > Changing the global vars to size_t seems mildly bogus to me. All it's
> > achieving is to use slightly more memory. It also just seems unrelated to the
> > change.
>
> I took a closer look at this. I agree that changing PqSendBufferSize
> to size_t is unnecessary: given the locations that it is used I see no
> risk of overflow anywhere. Changing the type of PqSendPointer and
> PqSendStart is needed though, because (as described by Heiki and David
> upthread) the argument type of internal_flush_buffer is size_t*. So if
> you actually pass int* there, and the sizes are not the same then you
> will start writing out of bounds. And because internal_flush_buffer is
> introduced in this patch, it is related to this change.
>
> This is what David just committed too.
>
> However, the "required" var actually should be of size_t to avoid
> overflow if len is larger than int even without this change. So
> attached is a tiny patch that does that.
Looking at the code in socket_putmessage_noblock(), I don't understand
why it's ok for PqSendBufferSize to be int but "required" must be
size_t. There's a line that does "PqSendBufferSize = required;". It
kinda looks like they both should be size_t. Am I missing something
that you've thought about?
David