Re: [PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.

[Jeff King <peff@peff.net>]

On Tue, May 07, 2019 at 04:58:32PM +0200, SZEDER Gábor wrote:

> On Wed, May 01, 2019 at 05:56:35PM +0900, Mike Hommey wrote:
> > The fread/fwrite-like functions in http.c, namely fread_buffer,
> > fwrite_buffer, fwrite_null, fwrite_sha1_file all return the
> > multiplication of the size and number of items they are being given.
> > 
> > Practically speaking, it doesn't matter, because in all contexts where
> > those functions are used, size is 1.
> > 
> > But those functions being similar to fread and fwrite (the curl API is
> > designed around being able to use fread and fwrite directly), it might
> > be preferable to make them behave like fread and fwrite, which, from
> > the fread/fwrite manual page, is:
> >    On  success, fread() and fwrite() return the number of items read
> >    or written.  This number equals the number of bytes transferred
> >    only when size is 1.  If an error occurs, or the end of the file
> >    is reached, the return value is a short item count (or zero).
> 
> This patch breaks the test 'push to remote repository with packed
> refs' in 't5540-http-push-webdav.sh':
> 
>   https://travis-ci.org/git/git/jobs/529223857#L2603
> 
> That test makes Apache spin like crazy at 100% CPU usage for about
> 30secs, after which, according to 'error.log':
> 
>   [Tue May 07 14:50:55.555166 2019] [mpm_prefork:notice] [pid 12638]
> AH00169: caught SIGTERM, shutting down

Yeah, this reproduces easily. The problem is that fread_buffer()
modifies "size" (if there are not enough bytes in the buffer to read),
so we cannot just assume it is "eltsize * nmemb" anymore.

I.e., we need to squash in:

diff --git a/http.c b/http.c
index 8dbc91f607..27aa0a3192 100644
--- a/http.c
+++ b/http.c
@@ -176,7 +176,7 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 	memcpy(ptr, buffer->buf.buf + buffer->posn, size);
 	buffer->posn += size;
 
-	return nmemb;
+	return size / eltsize;
 }
 
 #ifndef NO_CURL_IOCTL

The other conversions all look correct (there's a similar case in
fwrite_sha1_file, but it already does the division correctly).

-Peff