Re: [PATCH v6 10/12] http: replace unsafe size_t multiplication with st_mult

[Victoria Dye <vdye@github.com>]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> 
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Replace direct multiplication of two size_t parameters in curl response
>> stream handling callback functions with `st_mult` to guard against
>> overflows.
>>
>> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
>> ---
>>  http.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> 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)
>> @@ -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);
> 
> This is a really worthwhile fix, but shouldn't this be split into its
> own stand-alone patch? It applies on "master", and seems like something
> that's a good idea outside of this "test-http-server" topic.

While it's this change *can* stand alone, please keep in mind that
suggestions like this (recommending a series be split and resubmitted) can
be highly disruptive to the in-flight topic and the original contributor.

Monitoring and iterating on multiple series at once is time-consuming for
the contributor and reviewers, and often (although not in this case) it
creates a dependency of one series on another, which comes with a cost to
the maintainer's time. Not to say those recommendations should never be made
(e.g. in a clearly too-long series early in its review cycle, or when
certain patches lead to excessive context switching while reviewing), just
that they should be made more carefully, with consideration for the time of
other contributors.

So, with that in mind, I don't think this patch is critical enough to
separate into an independent submission, and (subjectively) it does not
disrupt the flow of this series.