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

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

[Tay Ray Chuan]

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(-)

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

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

[Sverre Rabbelier]

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

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

[Tay Ray Chuan]

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

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

[Johannes Schindelin]

Hi,

On Mon, 23 Nov 2009, Tay Ray Chuan wrote:

> 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

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

[Tay Ray Chuan]

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.

> 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

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

[Shawn O. Pearce]

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.

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

[Tay Ray Chuan]

Hi,

On Tue, Nov 24, 2009 at 5:04 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> 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