Re: [PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.
- Date: Tue, 7 May 2019 17:46:58 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.
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':
> 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
@@ -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;
The other conversions all look correct (there's a similar case in
fwrite_sha1_file, but it already does the division correctly).