{"thread":{"id":"21719","subject":"[PATCH 2/2] remote-curl.c: fix rpc_out()","startedAt":"2009-11-23T03:03:38Z","lastAt":"2009-11-24T01:35:51Z","messageCount":7,"participants":["Tay Ray Chuan","Sverre Rabbelier","Johannes Schindelin","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"128146","messageId":"20091123110338.2b230359.rctay89@gmail.com","threadId":"21719","inReplyTo":null,"subject":"[PATCH 2/2] remote-curl.c: fix rpc_out()","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-11-23T03:03:38Z","receivedAt":"2009-11-23T03:03:38Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Use comparisons between rpc->len and rpc->pos, rather than computing\ntheir difference. This avoids potential errors when this value is\nnegative and we access it.\n\nUse an int to store the return value from packet_read_line(), instead\nof a size_t.\n\nHandle the errorneous condition where rpc->pos exceeds rpc->len by\nprinting a message and aborting the transfer (return 0).\n\nRemove extraneous semicolon (';') at the end of the if statement, that\nprevented code in its block from executing.\n\nSigned-off-by: Tay Ray Chuan \n---\n\n Users might experience issues when pushing with chunked encoding when\n size_t avail is negative.\n\n remote-curl.c | 17 +++++++++++------\n 1 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 69eaf58..a2b8bbf 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -297,17 +297,22 @@ static size_t rpc_out(void *ptr, size_t eltsize,\n {\n \tsize_t max = eltsize * nmemb;\n \tstruct rpc_state *rpc = buffer_;\n-\tsize_t avail = rpc->len - rpc->pos;\n+\tsize_t avail = (size_t) 0;\n\n-\tif (!avail) {\n-\t\tavail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);\n-\t\tif (!avail)\n+\tif (rpc->pos == rpc->len) {\n+\t\tint n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);\n+\t\tif (!n)\n \t\t\treturn 0;\n \t\trpc->pos = 0;\n-\t\trpc->len = avail;\n+\t\tavail = rpc->len = (size_t) n;\n+\t} else if (rpc->pos > rpc->len) {\n+\t\terror(\"bad condition!\");\n+\t\treturn 0;\n+\t} else {\n+\t\tavail = rpc->len - rpc->pos;\n \t}\n\n-\tif (max < avail);\n+\tif (max < avail)\n \t\tavail = max;\n \tmemcpy(ptr, rpc->buf + rpc->pos, avail);\n \trpc->pos += avail;\n--\n1.6.5.3.301.gb2eb\n"},{"id":"128151","messageId":"fabb9a1e0911222153n633ade94w179513d4aa42a3d4@mail.gmail.com","threadId":"21719","inReplyTo":"20091123110338.2b230359.rctay89@gmail.com","subject":"Re: [PATCH 2/2] remote-curl.c: fix rpc_out()","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-11-23T05:53:18Z","receivedAt":"2009-11-23T05:53:18Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Nov 23, 2009 at 04:03, Tay Ray Chuan wrote:\n>  remote-curl.c |   17 +++++++++++------\n>  1 files changed, 11 insertions(+), 6 deletions(-)\n\nSeems like this type of patch would do very well with a test case or two?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"128158","messageId":"be6fef0d0911230038y13c23e3ek5591ee0dc3eaa47a@mail.gmail.com","threadId":"21719","inReplyTo":"fabb9a1e0911222153n633ade94w179513d4aa42a3d4@mail.gmail.com","subject":"Re: [PATCH 2/2] remote-curl.c: fix rpc_out()","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-11-23T08:38:09Z","receivedAt":"2009-11-23T08:38:09Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Mon, Nov 23, 2009 at 1:53 PM, Sverre Rabbelier wrote:\n> Seems like this type of patch would do very well with a test case or two?\n\nah, but to trigger the code involved, a sufficiently large test\nrepository is needed. The git repository would be enough. :)\n\nAny idea on how I could go about accessing this repository?\n\n-- \nCheers,\nRay Chuan\n"},{"id":"128165","messageId":"alpine.DEB.1.00.0911231137170.4985@pacific.mpi-cbg.de","threadId":"21719","inReplyTo":"be6fef0d0911230038y13c23e3ek5591ee0dc3eaa47a@mail.gmail.com","subject":"Re: [PATCH 2/2] remote-curl.c: fix rpc_out()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-11-23T10:39:31Z","receivedAt":"2009-11-23T10:39:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Nov 2009, Tay Ray Chuan wrote:\n\n> On Mon, Nov 23, 2009 at 1:53 PM, Sverre Rabbelier \n> wrote:\n> > Seems like this type of patch would do very well with a test case or \n> > two?\n> \n> ah, but to trigger the code involved, a sufficiently large test \n> repository is needed. The git repository would be enough. :)\n\nI guess you meant \"not be enough\", as an int can hold a pretty large \nnumber until it turns negative.\n\nSo I think in this case it is more harm- than helpful to have a test case.\n\nFor future reference: if you need a repository with special featurs \nfor testing, it is best to generate it in a test script (see the many test \ncases labeled 'setup' in our test suite for examples).\n\nCiao,\nDscho\n"},{"id":"128185","messageId":"20091124015415.c22d07c1.rctay89@gmail.com","threadId":"21719","inReplyTo":"alpine.DEB.1.00.0911231137170.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/2] remote-curl.c: fix rpc_out()","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-11-23T17:54:15Z","receivedAt":"2009-11-23T17:54:15Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Mon, Nov 23, 2009 at 6:39 PM, Johannes Schindelin wrote:\n> I guess you meant \"not be enough\", as an int can hold a pretty large\n> number until it turns negative.\n\nI really did mean 'enough' - enough to trigger the use of chunked\nencoding. I think the most important fix here is forcing rpc_out to\n(according to curl) memcpy at most size_t max bytes (the removal of the\nextraneous ';' addresses this). Pushing with chunked transfer would\nfail with this extra semicolon.\n\nRemoving the possibility of a negative size_t was a preventive measure,\nand, like you mentioned, requires a larger repository, so it's harder\nto test for.\n\nI probably should separate these issues into separate patches.\n\n> So I think in this case it is more harm- than helpful to have a test case.\n>\n> For future reference: if you need a repository with special featurs\n> for testing, it is best to generate it in a test script (see the many test\n> cases labeled 'setup' in our test suite for examples).\n\nHere's what I came up with: use the git repository which fetched the\ntest suite, and use the environment variable GIT_REMOTE_REFSPEC to specify\nthe remote refspec which the tester fetches git from.\n\n\n if test -z \"$GIT_REMOTE_REFSPEC\"; then\n \tsay 'skipping test, the remote for git is not specified'\n else\n \ttest_expect_success 'push with chunked encoding' '\n \t\tOWD=$(pwd) &&\n \t\tcd $TEST_DIRECTORY/../.git/ &&\n \t\tREPO=$(pwd) &&\n \t\tcd \"$OWD\" &&\n \t\techo \"$REPO\"/objects > .git/objects/info/alternates &&\n \t\tgit fetch \"$REPO\" \"$GIT_REMOTE_REFSPEC\"/*:refs/remotes/git/* &&\n \t\tgit push -v -v origin \"refs/remotes/git/*:refs/remotes/git/*\" \\\n \t\t\t>out 2>&1 &&\n \t\tgrep \"POST git-receive-pack (chunked)\" out\n \t'\n fi\n\nThoughts?\n\n-- \nCheers,\nRay Chuan\n"},{"id":"128204","messageId":"20091123210415.GH11919@spearce.org","threadId":"21719","inReplyTo":"20091123110338.2b230359.rctay89@gmail.com","subject":"Re: [PATCH 2/2] remote-curl.c: fix rpc_out()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-11-23T21:04:15Z","receivedAt":"2009-11-23T21:04:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If I understand the change right, the only thing that really matters\nhere is removing the extra ';' in the if (max < avail) condition.\n\nThat bug was the only reason why rpc->len < rpc->pos, and is thus\nthe only reason why avail would \"go negative\" (which it can't do\nsince its unsigned, it just wrapped around to a massive value).\n\nTay Ray Chuan wrote:\n> Use comparisons between rpc->len and rpc->pos, rather than computing\n> their difference. This avoids potential errors when this value is\n> negative and we access it.\n\nI don't see this as being very relevant here.\n \n> Use an int to store the return value from packet_read_line(), instead\n> of a size_t.\n\nWhy? The code is actually harder to follow. packet_read_line does\nnot return a negative value.\n\n> Handle the errorneous condition where rpc->pos exceeds rpc->len by\n> printing a message and aborting the transfer (return 0).\n\nThis condition should be impossible. It only occurred because of\nthe bug you were trying to fix, which is this one next item:\n \n> Remove extraneous semicolon (';') at the end of the if statement, that\n> prevented code in its block from executing.\n\nRight. This bug is really bad and should be fixed. But the message\nabove make this some little after-thought and doesn't explain what\nwas wrong with this being here.\n \n> ---\n> \n> Users might experience issues when pushing with chunked encoding when\n> size_t avail is negative.\n\nShouldn't this be in the commit message? Or something about how\npushing with chunked encoding was broken if the push was larger\nthan the default buffer size of 1 MiB?\n \n-- \nShawn.\n"},{"id":"128221","messageId":"be6fef0d0911231735g416d028al1821668be295fe6a@mail.gmail.com","threadId":"21719","inReplyTo":"20091123210415.GH11919@spearce.org","subject":"Re: [PATCH 2/2] remote-curl.c: fix rpc_out()","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-11-24T01:35:51Z","receivedAt":"2009-11-24T01:35:51Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Tue, Nov 24, 2009 at 5:04 AM, Shawn O. Pearce wrote:\n> If I understand the change right, the only thing that really matters\n> here is removing the extra ';' in the if (max < avail) condition.\n>\n> That bug was the only reason why rpc->len < rpc->pos, and is thus\n> the only reason why avail would \"go negative\" (which it can't do\n> since its unsigned, it just wrapped around to a massive value).\n> [snip]\n> Right. This bug is really bad and should be fixed. But the message\n> above make this some little after-thought and doesn't explain what\n> was wrong with this being here.\n\npoint noted.\n\n-- \nCheers,\nRay Chuan\n"}]}