{"thread":{"id":"51018","subject":"[PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.","startedAt":"2019-05-01T09:28:34Z","lastAt":"2019-05-07T23:04:00Z","messageCount":5,"participants":["Mike Hommey","Jeff King","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"374740","messageId":"20190501085635.7125-1-mh@glandium.org","threadId":"51018","inReplyTo":null,"subject":"[PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-01T08:56:35Z","receivedAt":"2019-05-01T09:28:34Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The fread/fwrite-like functions in http.c, namely fread_buffer,\nfwrite_buffer, fwrite_null, fwrite_sha1_file all return the\nmultiplication of the size and number of items they are being given.\n\nPractically speaking, it doesn't matter, because in all contexts where\nthose functions are used, size is 1.\n\nBut those functions being similar to fread and fwrite (the curl API is\ndesigned around being able to use fread and fwrite directly), it might\nbe preferable to make them behave like fread and fwrite, which, from\nthe fread/fwrite manual page, is:\n   On  success, fread() and fwrite() return the number of items read\n   or written.  This number equals the number of bytes transferred\n   only when size is 1.  If an error occurs, or the end of the file\n   is reached, the return value is a short item count (or zero).\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n http.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 98fb06df0b..8dbc91f607 100644\n--- a/http.c\n+++ b/http.c\n@@ -176,7 +176,7 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \tmemcpy(ptr, buffer->buf.buf + buffer->posn, size);\n \tbuffer->posn += size;\n \n-\treturn size;\n+\treturn nmemb;\n }\n \n #ifndef NO_CURL_IOCTL\n@@ -204,12 +204,12 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \tstruct strbuf *buffer = buffer_;\n \n \tstrbuf_add(buffer, ptr, size);\n-\treturn size;\n+\treturn nmemb;\n }\n \n size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)\n {\n-\treturn eltsize * nmemb;\n+\treturn nmemb;\n }\n \n static void closedown_active_slot(struct active_request_slot *slot)\n@@ -2319,14 +2319,14 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n \t\t\tBUG(\"curl_easy_getinfo for HTTP code failed: %s\",\n \t\t\t\tcurl_easy_strerror(c));\n \t\tif (slot->http_code >= 300)\n-\t\t\treturn size;\n+\t\t\treturn nmemb;\n \t}\n \n \tdo {\n \t\tssize_t retval = xwrite(freq->localfile,\n \t\t\t\t\t(char *) ptr + posn, size - posn);\n \t\tif (retval < 0)\n-\t\t\treturn posn;\n+\t\t\treturn posn / eltsize;\n \t\tposn += retval;\n \t} while (posn < size);\n \n@@ -2339,7 +2339,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n \t\tthe_hash_algo->update_fn(&freq->c, expn,\n \t\t\t\t\t sizeof(expn) - freq->stream.avail_out);\n \t} while (freq->stream.avail_in && freq->zret == Z_OK);\n-\treturn size;\n+\treturn nmemb;\n }\n \n struct http_object_request *new_http_object_request(const char *base_url,\n-- \n2.21.0\n\n"},{"id":"374778","messageId":"20190501184403.GG4109@sigill.intra.peff.net","threadId":"51018","inReplyTo":"20190501085635.7125-1-mh@glandium.org","subject":"Re: [PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-01T18:44:03Z","receivedAt":"2019-05-01T18:44:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 01, 2019 at 05:56:35PM +0900, Mike Hommey wrote:\n\n> The fread/fwrite-like functions in http.c, namely fread_buffer,\n> fwrite_buffer, fwrite_null, fwrite_sha1_file all return the\n> multiplication of the size and number of items they are being given.\n> \n> Practically speaking, it doesn't matter, because in all contexts where\n> those functions are used, size is 1.\n\nWow, this is a long-standing bug. :)\n\nI think it's further confused by curl's documentation. E.g.,\nCURLOPT_WRITEFUNCTION says:\n\n  Your callback should return the number of bytes actually taken care of.\n\nTo me, \"number of bytes\" implies implies size * nmemb. But earlier it\nsays:\n\n  ptr points to the delivered data, and the size of that data is nmemb;\n  size is always 1.\n\nSo I think they just use \"nmemb\" and \"bytes\" interchangeably. Which is\ncorrect, but misleading if you didn't read the whole thing.\n\nAt any rate, I think your patch is worth applying. It should have no\nimpact when used with curl, and is one less gotcha for somebody trying\nto use these elsewhere.\n\nAnother option would be to actually assert(eltsize == 1), and then just\nuse nmemb consistently. That might make the logic a little easier to\nfollow, but I doubt it matters much either way.\n\n-Peff\n"},{"id":"375035","messageId":"20190507145832.GN14763@szeder.dev","threadId":"51018","inReplyTo":"20190501085635.7125-1-mh@glandium.org","subject":"Re: [PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-05-07T14:58:32Z","receivedAt":"2019-05-07T14:58:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, May 01, 2019 at 05:56:35PM +0900, Mike Hommey wrote:\n> The fread/fwrite-like functions in http.c, namely fread_buffer,\n> fwrite_buffer, fwrite_null, fwrite_sha1_file all return the\n> multiplication of the size and number of items they are being given.\n> \n> Practically speaking, it doesn't matter, because in all contexts where\n> those functions are used, size is 1.\n> \n> But those functions being similar to fread and fwrite (the curl API is\n> designed around being able to use fread and fwrite directly), it might\n> be preferable to make them behave like fread and fwrite, which, from\n> the fread/fwrite manual page, is:\n>    On  success, fread() and fwrite() return the number of items read\n>    or written.  This number equals the number of bytes transferred\n>    only when size is 1.  If an error occurs, or the end of the file\n>    is reached, the return value is a short item count (or zero).\n\nThis patch breaks the test 'push to remote repository with packed\nrefs' in 't5540-http-push-webdav.sh':\n\n  https://travis-ci.org/git/git/jobs/529223857#L2603\n\nThat test makes Apache spin like crazy at 100% CPU usage for about\n30secs, after which, according to 'error.log':\n\n  [Tue May 07 14:50:55.555166 2019] [mpm_prefork:notice] [pid 12638]\nAH00169: caught SIGTERM, shutting down\n\n\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  http.c | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/http.c b/http.c\n> index 98fb06df0b..8dbc91f607 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -176,7 +176,7 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n>  \tmemcpy(ptr, buffer->buf.buf + buffer->posn, size);\n>  \tbuffer->posn += size;\n>  \n> -\treturn size;\n> +\treturn nmemb;\n>  }\n>  \n>  #ifndef NO_CURL_IOCTL\n> @@ -204,12 +204,12 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n>  \tstruct strbuf *buffer = buffer_;\n>  \n>  \tstrbuf_add(buffer, ptr, size);\n> -\treturn size;\n> +\treturn nmemb;\n>  }\n>  \n>  size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)\n>  {\n> -\treturn eltsize * nmemb;\n> +\treturn nmemb;\n>  }\n>  \n>  static void closedown_active_slot(struct active_request_slot *slot)\n> @@ -2319,14 +2319,14 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n>  \t\t\tBUG(\"curl_easy_getinfo for HTTP code failed: %s\",\n>  \t\t\t\tcurl_easy_strerror(c));\n>  \t\tif (slot->http_code >= 300)\n> -\t\t\treturn size;\n> +\t\t\treturn nmemb;\n>  \t}\n>  \n>  \tdo {\n>  \t\tssize_t retval = xwrite(freq->localfile,\n>  \t\t\t\t\t(char *) ptr + posn, size - posn);\n>  \t\tif (retval < 0)\n> -\t\t\treturn posn;\n> +\t\t\treturn posn / eltsize;\n>  \t\tposn += retval;\n>  \t} while (posn < size);\n>  \n> @@ -2339,7 +2339,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n>  \t\tthe_hash_algo->update_fn(&freq->c, expn,\n>  \t\t\t\t\t sizeof(expn) - freq->stream.avail_out);\n>  \t} while (freq->stream.avail_in && freq->zret == Z_OK);\n> -\treturn size;\n> +\treturn nmemb;\n>  }\n>  \n>  struct http_object_request *new_http_object_request(const char *base_url,\n> -- \n> 2.21.0\n> \n"},{"id":"375058","messageId":"20190507214658.GC19955@sigill.intra.peff.net","threadId":"51018","inReplyTo":"20190507145832.GN14763@szeder.dev","subject":"Re: [PATCH] Make fread/fwrite-like functions in http.c more like fread/fwrite.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-07T21:46:58Z","receivedAt":"2019-05-07T21:47:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 07, 2019 at 04:58:32PM +0200, SZEDER Gábor wrote:\n\n> On Wed, May 01, 2019 at 05:56:35PM +0900, Mike Hommey wrote:\n> > The fread/fwrite-like functions in http.c, namely fread_buffer,\n> > fwrite_buffer, fwrite_null, fwrite_sha1_file all return the\n> > multiplication of the size and number of items they are being given.\n> > \n> > Practically speaking, it doesn't matter, because in all contexts where\n> > those functions are used, size is 1.\n> > \n> > But those functions being similar to fread and fwrite (the curl API is\n> > designed around being able to use fread and fwrite directly), it might\n> > be preferable to make them behave like fread and fwrite, which, from\n> > the fread/fwrite manual page, is:\n> >    On  success, fread() and fwrite() return the number of items read\n> >    or written.  This number equals the number of bytes transferred\n> >    only when size is 1.  If an error occurs, or the end of the file\n> >    is reached, the return value is a short item count (or zero).\n> \n> This patch breaks the test 'push to remote repository with packed\n> refs' in 't5540-http-push-webdav.sh':\n> \n>   https://travis-ci.org/git/git/jobs/529223857#L2603\n> \n> That test makes Apache spin like crazy at 100% CPU usage for about\n> 30secs, after which, according to 'error.log':\n> \n>   [Tue May 07 14:50:55.555166 2019] [mpm_prefork:notice] [pid 12638]\n> AH00169: caught SIGTERM, shutting down\n\nYeah, this reproduces easily. The problem is that fread_buffer()\nmodifies \"size\" (if there are not enough bytes in the buffer to read),\nso we cannot just assume it is \"eltsize * nmemb\" anymore.\n\nI.e., we need to squash in:\n\ndiff --git a/http.c b/http.c\nindex 8dbc91f607..27aa0a3192 100644\n--- a/http.c\n+++ b/http.c\n@@ -176,7 +176,7 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \tmemcpy(ptr, buffer->buf.buf + buffer->posn, size);\n \tbuffer->posn += size;\n \n-\treturn nmemb;\n+\treturn size / eltsize;\n }\n \n #ifndef NO_CURL_IOCTL\n\nThe other conversions all look correct (there's a similar case in\nfwrite_sha1_file, but it already does the division correctly).\n\n-Peff\n"},{"id":"375066","messageId":"20190507230354.28638-1-mh@glandium.org","threadId":"51018","inReplyTo":"20190507214658.GC19955@sigill.intra.peff.net","subject":"[PATCH v2] Make fread/fwrite-like functions in http.c more like fread/fwrite.","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-07T23:03:54Z","receivedAt":"2019-05-07T23:04:00Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The fread/fwrite-like functions in http.c, namely fread_buffer,\nfwrite_buffer, fwrite_null, fwrite_sha1_file all return the\nmultiplication of the size and number of items they are being given.\n\nPractically speaking, it doesn't matter, because in all contexts where\nthose functions are used, size is 1.\n\nBut those functions being similar to fread and fwrite (the curl API is\ndesigned around being able to use fread and fwrite directly), it might\nbe preferable to make them behave like fread and fwrite, which, from\nthe fread/fwrite manual page, is:\n   On  success, fread() and fwrite() return the number of items read\n   or written.  This number equals the number of bytes transferred\n   only when size is 1.  If an error occurs, or the end of the file\n   is reached, the return value is a short item count (or zero).\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n http.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 98fb06df0b..27aa0a3192 100644\n--- a/http.c\n+++ b/http.c\n@@ -176,7 +176,7 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \tmemcpy(ptr, buffer->buf.buf + buffer->posn, size);\n \tbuffer->posn += size;\n \n-\treturn size;\n+\treturn size / eltsize;\n }\n \n #ifndef NO_CURL_IOCTL\n@@ -204,12 +204,12 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \tstruct strbuf *buffer = buffer_;\n \n \tstrbuf_add(buffer, ptr, size);\n-\treturn size;\n+\treturn nmemb;\n }\n \n size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)\n {\n-\treturn eltsize * nmemb;\n+\treturn nmemb;\n }\n \n static void closedown_active_slot(struct active_request_slot *slot)\n@@ -2319,14 +2319,14 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n \t\t\tBUG(\"curl_easy_getinfo for HTTP code failed: %s\",\n \t\t\t\tcurl_easy_strerror(c));\n \t\tif (slot->http_code >= 300)\n-\t\t\treturn size;\n+\t\t\treturn nmemb;\n \t}\n \n \tdo {\n \t\tssize_t retval = xwrite(freq->localfile,\n \t\t\t\t\t(char *) ptr + posn, size - posn);\n \t\tif (retval < 0)\n-\t\t\treturn posn;\n+\t\t\treturn posn / eltsize;\n \t\tposn += retval;\n \t} while (posn < size);\n \n@@ -2339,7 +2339,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n \t\tthe_hash_algo->update_fn(&freq->c, expn,\n \t\t\t\t\t sizeof(expn) - freq->stream.avail_out);\n \t} while (freq->stream.avail_in && freq->zret == Z_OK);\n-\treturn size;\n+\treturn nmemb;\n }\n \n struct http_object_request *new_http_object_request(const char *base_url,\n-- \n2.21.0\n\n"}]}