threads / patch / 21719

patch, 2 partsremote-curl.c: fix rpc_out()

Subject: [PATCH 2/2] remote-curl.c: fix rpc_out()

## tl;dr

7 messages between Nov 23, 2009 and Nov 24, 2009. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Tay Ray Chuan· Nov 23, 2009, 03:03 UTC · lore

Use comparisons between rpc->len and rpc->pos, rather than computing their difference. This avoids potential errors when this value is negative and we access it.

Use an int to store the return value from packet_read_line(), instead of a size_t.

Handle the errorneous condition where rpc->pos exceeds rpc->len by printing a message and aborting the transfer (return 0).

Remove extraneous semicolon (';') at the end of the if statement, that prevented code in its block from executing.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
  Users might experience issues when pushing with chunked encoding when
  size_t avail is negative.
 remote-curl.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)
Show changes to remote-curl.c +11 −7
diff --git a/remote-curl.c b/remote-curl.c
index 69eaf58..a2b8bbf 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -297,17 +297,22 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 {
 	size_t max = eltsize * nmemb;
 	struct rpc_state *rpc = buffer_;
-	size_t avail = rpc->len - rpc->pos;
+	size_t avail = (size_t) 0;

-	if (!avail) {
-		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
-		if (!avail)
+	if (rpc->pos == rpc->len) {
+		int n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		if (!n)
 			return 0;
 		rpc->pos = 0;
-		rpc->len = avail;
+		avail = rpc->len = (size_t) n;
+	} else if (rpc->pos > rpc->len) {
+		error("bad condition!");
+		return 0;
+	} else {
+		avail = rpc->len - rpc->pos;
 	}

-	if (max < avail);
+	if (max < avail)
 		avail = max;
 	memcpy(ptr, rpc->buf + rpc->pos, avail);
 	rpc->pos += avail;
--
1.6.5.3.301.gb2eb
Sverre Rabbelier· Nov 23, 2009, 05:53 UTC · re: Tay Ray Chuan · lore

Re: [PATCH 2/2] remote-curl.c: fix rpc_out()

Heya,
On Mon, Nov 23, 2009 at 04:03, Tay Ray Chuan <rctay89@gmail.com> wrote:
>  remote-curl.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
Seems like this type of patch would do very well with a test case or two?
-- 
Cheers,

Sverre Rabbelier
Tay Ray Chuan· Nov 23, 2009, 08:38 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 2/2] remote-curl.c: fix rpc_out()

Hi,
On Mon, Nov 23, 2009 at 1:53 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Seems like this type of patch would do very well with a test case or two?

ah, but to trigger the code involved, a sufficiently large test repository is needed. The git repository would be enough. :)

Any idea on how I could go about accessing this repository?
-- 
Cheers,
Ray Chuan
Johannes Schindelin· Nov 23, 2009, 10:39 UTC · re: Tay Ray Chuan · lore

Re: [PATCH 2/2] remote-curl.c: fix rpc_out()

Hi,
On Mon, 23 Nov 2009, Tay Ray Chuan wrote:
Show 7 quoted lines
> On Mon, Nov 23, 2009 at 1:53 PM, Sverre Rabbelier <srabbelier@gmail.com> 
> wrote:
> > Seems like this type of patch would do very well with a test case or 
> > two?
> 
> ah, but to trigger the code involved, a sufficiently large test 
> repository is needed. The git repository would be enough. :)

I guess you meant "not be enough", as an int can hold a pretty large number until it turns negative.

So I think in this case it is more harm- than helpful to have a test case.

For future reference: if you need a repository with special featurs for testing, it is best to generate it in a test script (see the many test cases labeled 'setup' in our test suite for examples).

Ciao, Dscho

Tay Ray Chuan· Nov 23, 2009, 17:54 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] remote-curl.c: fix rpc_out()

Hi,
On Mon, Nov 23, 2009 at 6:39 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> I guess you meant "not be enough", as an int can hold a pretty large
> number until it turns negative.

I really did mean 'enough' - enough to trigger the use of chunked encoding. I think the most important fix here is forcing rpc_out to (according to curl) memcpy at most size_t max bytes (the removal of the extraneous ';' addresses this). Pushing with chunked transfer would fail with this extra semicolon.

Removing the possibility of a negative size_t was a preventive measure, and, like you mentioned, requires a larger repository, so it's harder to test for.

I probably should separate these issues into separate patches.
Show 5 quoted lines
> So I think in this case it is more harm- than helpful to have a test case.
>
> For future reference: if you need a repository with special featurs
> for testing, it is best to generate it in a test script (see the many test
> cases labeled 'setup' in our test suite for examples).

Here's what I came up with: use the git repository which fetched the test suite, and use the environment variable GIT_REMOTE_REFSPEC to specify the remote refspec which the tester fetches git from.

  if test -z "$GIT_REMOTE_REFSPEC"; then
  	say 'skipping test, the remote for git is not specified'
  else
  	test_expect_success 'push with chunked encoding' '
  		OWD=$(pwd) &&
  		cd $TEST_DIRECTORY/../.git/ &&
  		REPO=$(pwd) &&
  		cd "$OWD" &&
  		echo "$REPO"/objects > .git/objects/info/alternates &&
  		git fetch "$REPO" "$GIT_REMOTE_REFSPEC"/*:refs/remotes/git/* &&
  		git push -v -v origin "refs/remotes/git/*:refs/remotes/git/*" \
  			>out 2>&1 &&
  		grep "POST git-receive-pack (chunked)" out
  	'
  fi
Thoughts?
-- 
Cheers,
Ray Chuan
Shawn O. Pearce· Nov 23, 2009, 21:04 UTC · re: Tay Ray Chuan · lore

Re: [PATCH 2/2] remote-curl.c: fix rpc_out()

If I understand the change right, the only thing that really matters here is removing the extra ';' in the if (max < avail) condition.

That bug was the only reason why rpc->len < rpc->pos, and is thus the only reason why avail would "go negative" (which it can't do since its unsigned, it just wrapped around to a massive value).

Tay Ray Chuan <rctay89@gmail.com> wrote:
> Use comparisons between rpc->len and rpc->pos, rather than computing
> their difference. This avoids potential errors when this value is
> negative and we access it.
I don't see this as being very relevant here.
 
> Use an int to store the return value from packet_read_line(), instead
> of a size_t.

Why? The code is actually harder to follow. packet_read_line does not return a negative value.

> Handle the errorneous condition where rpc->pos exceeds rpc->len by
> printing a message and aborting the transfer (return 0).
This condition should be impossible.  It only occurred because of
the bug you were trying to fix, which is this one next item:
 
> Remove extraneous semicolon (';') at the end of the if statement, that
> prevented code in its block from executing.
Right.  This bug is really bad and should be fixed.  But the message
above make this some little after-thought and doesn't explain what
was wrong with this being here.
 
> ---
> 
>   Users might experience issues when pushing with chunked encoding when
>   size_t avail is negative.
Shouldn't this be in the commit message?  Or something about how
pushing with chunked encoding was broken if the push was larger
than the default buffer size of 1 MiB?
 
-- 
Shawn.
Tay Ray Chuan· Nov 24, 2009, 01:35 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 2/2] remote-curl.c: fix rpc_out()

Hi,
On Tue, Nov 24, 2009 at 5:04 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 10 quoted lines
> If I understand the change right, the only thing that really matters
> here is removing the extra ';' in the if (max < avail) condition.
>
> That bug was the only reason why rpc->len < rpc->pos, and is thus
> the only reason why avail would "go negative" (which it can't do
> since its unsigned, it just wrapped around to a massive value).
> [snip]
> Right.  This bug is really bad and should be fixed.  But the message
> above make this some little after-thought and doesn't explain what
> was wrong with this being here.
point noted.
-- 
Cheers,
Ray Chuan

← back to recent threads