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

> Æ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.