On Wed, Jan 18 2023, Victoria Dye wrote:
> Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
>>
>>> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxx>
>>> ---
>>> 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.
Yes, I take your general point, it's not always the right thing,
sometimes a while-at-it cleanup is better than a split-out etc.
In this case the split-out seemed like it wouldn't create a dependency
between topics, as the rest of the series didn't rely on the overflow
sanity check being added, it's just a good idea to do it in general.
