| Message ID | cc9a220ed1f12aef2f4df940e71adc1fad917a6b.1674012618.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
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.
Æ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.
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@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. 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.
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);