{"thread":{"id":"11594","subject":"[PATCH decompress BUG] Fix decompress_next_from() wrong argument value","startedAt":"2008-01-11T20:47:04Z","lastAt":"2008-01-12T18:44:08Z","messageCount":6,"participants":["Marco Costalba","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"65057","messageId":"e5bfff550801111247l1ccf171ene5b53b8d6841a864@mail.gmail.com","threadId":"11594","inReplyTo":null,"subject":"[PATCH decompress BUG] Fix decompress_next_from() wrong argument value","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2008-01-11T20:47:04Z","receivedAt":"2008-01-11T20:47:04Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"Function decompress_next_from() needs a pointer to a buffer\nand the buffer size as arguments.\n\nInteresting enough the function fill() that returns the\nbuffer pointer happens to modify also the buffer size,\nstored in a variable at file scope.\n\nSo we need to guarantee fill() is called before to use buffer\nsize as argument in decompress_next_from()\n\nSigned-off-by: Marco Costalba \n---\nPatch to be applied above decompress helper series.\n\nNot to be pedantic, but have a function that gives two really\ncoupled values, as a buffer pointer and the size, the first as return\nvalue and the second through a variable at file scope is not something\nyou are going to see advertised in the programming books!\n\nSorry for this little rant but this bug really made me crazy.\n\nWith this patch 'make test' runs with success!\n\n\n builtin-unpack-objects.c | 3 ++-\n index-pack.c | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex f1a4883..72293ec 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -68,7 +68,8 @@ static void *get_data(unsigned long size)\n \tdecompress_into(&stream, buf, size);\n\n \tfor (;;) {\n-\t\tint ret = decompress_next_from(&stream, fill(1), len, Z_NO_FLUSH);\n+\t\tvoid* tmp = fill(1); // fill() modifies len, so be sure is evaluated as first\n+\t\tint ret = decompress_next_from(&stream, tmp, len, Z_NO_FLUSH);\n \t\tuse(len - stream.avail_in);\n \t\tif (stream.total_out == size && ret == Z_STREAM_END)\n \t\t\tbreak;\ndiff --git a/index-pack.c b/index-pack.c\nindex 30d7837..13b308d 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -173,7 +173,8 @@ static void *unpack_entry_data(unsigned long\noffset, unsigned long size)\n \tdecompress_into(&stream, buf, size);\n\n \tfor (;;) {\n-\t\tint ret = decompress_next_from(&stream, fill(1), input_len, Z_NO_FLUSH);\n+\t\tvoid* tmp = fill(1); // fill() modifies input_len, so be sure is\nevaluated as first\n+\t\tint ret = decompress_next_from(&stream, tmp, input_len, Z_NO_FLUSH);\n \t\tuse(input_len - stream.avail_in);\n \t\tif (stream.total_out == size && ret == Z_STREAM_END)\n \t\t\tbreak;\n-- \n1.5.4.rc2.95.g0eaa-dirty\n"},{"id":"65090","messageId":"7vfxx3290v.fsf@gitster.siamese.dyndns.org","threadId":"11594","inReplyTo":"e5bfff550801111247l1ccf171ene5b53b8d6841a864@mail.gmail.com","subject":"Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-12T00:16:16Z","receivedAt":"2008-01-12T00:16:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marco Costalba\" writes:\n\n> Patch to be applied above decompress helper series.\n\nNo way. That will mean that the resulting series will start\nwith a known bug.\n\n> Not to be pedantic, but have a function that gives two really\n> coupled values, as a buffer pointer and the size, the first as return\n> value and the second through a variable at file scope is not something\n> you are going to see advertised in the programming books!\n>\n> Sorry for this little rant but this bug really made me crazy.\n\nPardon me. Are you talking about a bug you introduced earlier\nin your own series that hasn't been applied (and you very well\nknow will not be until 1.5.4 is out, now we are deep in -rc\ncycle)?\n\nIf so, you did a great disservice to me by sounding as if you\nare blaming somebody else's existing bug. I wasted some time\nhunting for a non-existent bug in the code that is being readied\nfor 1.5.4 final for quite some time, in order to pick only the\nrelevant \"fix\" from your patch.\n\nIt turns out, luckily, existing code did not have such a bug.\nWhat a relief for the maintainer in bugfix-only freeze mode.\n\nNext time around, please mark the patch on the Subject: line to\nbe squashed to your earlier [PATCH 5/6] before [PATCH 6/6].\nThat will also solve the bisectability problem.\n\nAnyway, thanks. I was planning to queue the series in 'pu' or\n'next' after tagging -rc3, so not be silent and giving a proper\nfix was the right thing to do. My above rant is just about the\npresentation.\n"},{"id":"65137","messageId":"e5bfff550801112306g6b8127dft80484c9fd8554992@mail.gmail.com","threadId":"11594","inReplyTo":"7vfxx3290v.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2008-01-12T07:06:35Z","receivedAt":"2008-01-12T07:06:35Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On Jan 12, 2008 1:16 AM, Junio C Hamano wrote:\n>\n> Next time around, please mark the patch on the Subject: line to\n> be squashed to your earlier [PATCH 5/6] before [PATCH 6/6].\n>\n\nVery sorry for wasting your time I should have been more clear that it\nwas a bug in the new series. And of course this series is not to be\napplied to stable git.\n\nThe only two points in the current code in master that I would like to\nreport to you are a _possible_ missing inflateEnd() before a new\ninflateInit(), but I am not confident with that part of code to judge\nif is a bug or not, anyway that's the _possible_ diff.\n\ndiff --git a/http-push.c b/http-push.c\nindex 55d0c94..e0a4cc6 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -307,6 +307,7 @@ static void start_fetch_loose(struct\ntransfer_request *request)\n \t/* Reset inflate/SHA1 if there was an error reading the previous temp\n \t file; also rewind to the beginning of the local file. */\n \tif (prev_read == -1) {\n+\t\tinflateEnd(&request->stream);\n \t\tmemset(&request->stream, 0, sizeof(request->stream));\n \t\tinflateInit(&request->stream);\n \t\tSHA1_Init(&request->c);\ndiff --git a/http-walker.c b/http-walker.c\nindex 2c37868..a18067c 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -182,6 +182,7 @@ static void start_object_request(struct walker *walker,\n \t/* Reset inflate/SHA1 if there was an error reading the previous temp\n \t file; also rewind to the beginning of the local file. */\n \tif (prev_read == -1) {\n+\t\tinflateEnd(&obj_req->stream);\n \t\tmemset(&obj_req->stream, 0, sizeof(obj_req->stream));\n \t\tinflateInit(&obj_req->stream);\n \t\tSHA1_Init(&obj_req->c);\n\n\n\nI have not created a proper patch becuase I don't know if the missing\ninflateEnd(), it is a bug or not. The above diff it's just a way to\npoint you quickly and hopefully clearly to the interested code .\n\n\nSorry again for the trouble I had caused to you. For sure I will be\nmuch more careful in the future to be clear in the subjects. And also\nsorry for my rant but it was very late and I was tired after fighting\nwith that _my_ bug.\n\nMarco\n"},{"id":"65142","messageId":"7vir1zwlcw.fsf@gitster.siamese.dyndns.org","threadId":"11594","inReplyTo":"e5bfff550801112306g6b8127dft80484c9fd8554992@mail.gmail.com","subject":"Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-12T07:31:43Z","receivedAt":"2008-01-12T07:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marco Costalba\" writes:\n\n> The only two points in the current code in master that I would like to\n> report to you are a _possible_ missing inflateEnd() before a new\n> inflateInit(), but I am not confident with that part of code to judge\n> if is a bug or not, anyway that's the _possible_ diff.\n>\n> diff --git a/http-push.c b/http-push.c\n> index 55d0c94..e0a4cc6 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -307,6 +307,7 @@ static void start_fetch_loose(struct\n> transfer_request *request)\n> \t/* Reset inflate/SHA1 if there was an error reading the previous temp\n> \t file; also rewind to the beginning of the local file. */\n> \tif (prev_read == -1) {\n> +\t\tinflateEnd(&request->stream);\n> \t\tmemset(&request->stream, 0, sizeof(request->stream));\n> \t\tinflateInit(&request->stream);\n> \t\tSHA1_Init(&request->c);\n\nI think this could leak if request->stream already had\nsomething in it, but I do not see anything that is done to it\nafter it is initialized and the code reaches to this point.\n\nIn fact, I suspect that it would make more sense to remove the\nearlier memset() that clears request->stream and inflateInit(),\nand moving the memset() and inflateInit() we see above out of\nthat if clause (before checking prev_read).\n\nThe same comment applies to the other hunk.\n\nBy the way, I was looking at the earlier two series from you\n(compress and decompress), and noticed some of them were corrupt\nwith linewrap. As I think they are good clean-up patches, I'd\nlike to apply them as one of the first series post 1.5.4. As\nsuch, this request is not urgent at all, but please resend with\na clean-up when 'master'/'next' reopens.\n\nThanks.\n"},{"id":"65144","messageId":"e5bfff550801112342w4faee040nad294f3962160180@mail.gmail.com","threadId":"11594","inReplyTo":"7vir1zwlcw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2008-01-12T07:42:14Z","receivedAt":"2008-01-12T07:42:14Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On Jan 12, 2008 8:31 AM, Junio C Hamano wrote:\n>\n> By the way, I was looking at the earlier two series from you\n> (compress and decompress), and noticed some of them were corrupt\n> with linewrap. As I think they are good clean-up patches, I'd\n> like to apply them as one of the first series post 1.5.4. As\n> such, this request is not urgent at all, but please resend with\n> a clean-up when 'master'/'next' reopens.\n>\n\nSure.\n\nDo you prefer patches differently organized or I can keep the same\npatch contents (of course with squashing the bug fixes in) ?\n\nMarco\n"},{"id":"65202","messageId":"7v1w8mvq87.fsf@gitster.siamese.dyndns.org","threadId":"11594","inReplyTo":"e5bfff550801112342w4faee040nad294f3962160180@mail.gmail.com","subject":"Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-12T18:44:08Z","receivedAt":"2008-01-12T18:44:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marco Costalba\" writes:\n\n> Do you prefer patches differently organized or I can keep the same\n> patch contents (of course with squashing the bug fixes in) ?\n\nMy impression was that the organization was good (addition of\nthe helpers, and then conversion to existing code to use the\nhelper piece-by-piece), even though I admit that I did not look\nat them very deeply.\n"}]}