On Fri, Jan 20, 2023 at 10:08:48PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> Replace direct multiplication of two size_t parameters in curl response
> stream handling callback functions with `st_mult` to guard against
> overflows.
Hmm. So part of me says that more overflow detection is better than
less, but...I really doubt this is doing anything, and it feels odd to
me to do overflow checks when there is no allocation.
There are tons of integer multiplications in Git. Our usual strategy is
to try to handle overflow like this when we're about to allocate a
buffer, with the idea that we'll avoid a truncated size (that we may
later fill with too many bytes).
In these cases, we could possibly avoid a weird or wrong result due to
truncation, but I don't see how that is different than most of the rest
of Git. What makes these worth touching?
Moreover...
> @@ -176,7 +176,7 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
>
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> {
> - size_t size = eltsize * nmemb;
> + size_t size = st_mult(eltsize, nmemb);
> struct strbuf *buffer = buffer_;
>
> strbuf_add(buffer, ptr, size);
The caller is already claiming to have eltsize*nmemb bytes accessible
via "ptr". How did it get such a buffer if that overflows size_t?
> diff --git a/http.c b/http.c
> index 8a5ba3f4776..a2a80318bb2 100644
> --- a/http.c
> +++ b/http.c
> @@ -146,7 +146,7 @@ static int http_schannel_use_ssl_cainfo;
>
> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> {
> - size_t size = eltsize * nmemb;
> + size_t size = st_mult(eltsize, nmemb);
> struct buffer *buffer = buffer_;
>
> if (size > buffer->buf.len - buffer->posn)
Likewise the caller is asking us to fill a buffer that is eltsize*nmemb.
So they must have allocated it already. How can it be bigger than a
size_t?
In practice, of course, these are both coming from curl, and I strongly
suspect that curl always sets "1" for eltsize anyway, since it's working
with bytes. The two fields only exist to conform to the weird fread()
interface for historical reasons.
So I don't think this patch is really hurting much. It just feels like a
weird one-off that makes the code inconsistent. If somebody who was
wanting to write similar code later asks "why is this one st_mult(), but
not other multiplications", I wouldn't have an answer for them.
-Peff
