Re: Parsing a git HTTP protocol response

[Bryan Turner <bturner@atlassian.com>]

On Fri, Nov 30, 2018 at 5:05 PM Farhan Khan <khanzf@gmail.com> wrote:
>
> Hi all,
>
> I am writing an implementation of the git HTTP pack protocol in C. It
> just does a request to clone a repository. It works pretty well for
> small repositories, but seems to fail on larger repositories and I do
> not understand why.
>
> All that my code does is send a hard-coded "want" request. As I
> understand it, responses come with a four-byte size prefix, then the
> data of the size - 4. The first four bytes are the size of the object
> in ASCII and after that size, a new object should begin by specifying
> its size. The final "0000" string should signify the end.
>
> On small repositories my code works fine. But when use a large git
> repository, I hit an object size that cannot be interpreted by ASCII
> into a number. In particular, right before the crash I am pretty
> consistently getting a size starting with 0x32,0x00 or 0x32,0x30,0x00
> (note, it starts with 0x32 and has 0x00 in there). This is not a
> readable four byte ascii value, likely an erroneous size value, which
> causes the next object size calculation to land incorrectly and
> subsequently the program to malfunction.
>
> My questions:
> 1. Am I misunderstanding the protocol?
> 2. Does 0x32 signify something?
> 3. Also, where can I see in the source code git parses the data it
> receives from a "want" request?
>
> If you would like to see my code, it is located here:
> http://git.farhan.codes/farhan/post
> To compile to Linux run: gcc post.c main.c -lcurl -o post
> To compile on FreeBSD you can use the Makefile or: cc -L
> /usr/local/lib -I /usr/local/include -lcurl main.c post.c -o post
> In both cases you need to have libcurl installed.
>
> To run do: ./post [a git http url] [a commit value]
> ie: ./post http://git.farhan.codes/farhan/openbsd
> 373dd5ba116d42cddf1fdcb58b578a4274f6d589
>
> I have a lot of debugging printf() messages, but it eventually hits a
> point where you see this:
>
> ========Start=========
> Current stats: Current state: 999 Received: 1448
> We have enough bytes, moving forward
> == New Object
> No Error, object is 32 bytes
> Size bytes: 32 30 00 00

I modified your debugging output a little bit, and right before the
failure happens I see this in my output:

========Start=========
Current stats: Current state: 999 Received: 1298
Read complete; still missing 1296 bytes (6901 read of 8197)
Offset: 1298

========Start=========
Current stats: Current state: 999 Received: 1298
Read complete; 2 more bytes in buffer
== New Object
Size bytes: 32 30 00 2b

Based on that, it appears what's happening is you're not handling the
case where you end up with fewer than 4 bytes in the buffer. As a
result, memcpy reads past the end of the response buffer and gets
whatever it gets, resulting in an incorrect parsed size.

The while loop in pack_protocol_line is checking +1, but I think it
should be checking +3 to ensure there's at least 4 bytes so it can get
a complete size. The "parseread" struct will need to grow a couple
more fields to allow buffering some additional bytes between
pack_protocol_line calls (because curl requires the write function to
always consume the full buffer) and, at the start of the loop, when
reading/parsing a size, the code will need to consider any buffered
bytes from the previous function call. That requires some knock-on
changes to how the offset is handled, as well as the the initial
"psize" set when starting a new packet.

Once you start accounting for reads that cut off in 1, 2 or 3 bytes
into the 4 required to parse a size, your program should be able to
run to completion.

Here's a (very ugly) patch I threw together on top of your code:

diff --git a/main.c b/main.c
index 956362b..8873fd5 100644
--- a/main.c
+++ b/main.c
@@ -71,7 +71,7 @@ int main(int argc, char *argv[]) {
  curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list);
  curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, (long)content_length);
  curl_easy_setopt(curl, CURLOPT_WRITEDATA, &parseread);
- curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, pack_protocol_line);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &pack_protocol_line);

  res = curl_easy_perform(curl);
  if (curl != CURLE_OK)
diff --git a/post.c b/post.c
index cfffd5c..4f8512c 100644
--- a/post.c
+++ b/post.c
@@ -36,89 +36,83 @@ size_t pack_protocol_line(void *buffer, size_t
size, size_t nmemb, void *userp)
 {
  unsigned char *reply = buffer;
  struct parseread *parseread = userp;
- int offset = 0;
+ int offset = 0, pending = 0, remaining = 0;
  char tmp[5];
- int check;
-
- int pool = size * nmemb;

  printf("\n\n========Start=========\n");
- printf("Current stats: Current state: %d Received: %ld\n",
parseread->state, size*nmemb);
+ printf("Current stats: Current state: %d Received: %ld\n",
parseread->state, nmemb);

- while(offset + 1 < size + nmemb) {
+ while (offset + 3 < nmemb) {
  if (parseread->state == STATE_NEWLINE) {
  printf("== New Object\n");
  bzero(tmp, 5);
- memcpy(tmp, buffer+offset, 4); tmp[4] = '\0';
- check = sscanf(tmp, "%04x", &parseread->osize);

- if (parseread->osize == 0) {
- printf("Only happens if reached terminating 0000\n");
+ if (parseread->bsize > 0) {
+ // If there are buffered size bytes, consume them
+ memcpy(tmp, parseread->buffer, parseread->bsize);
+ memcpy(tmp + parseread->bsize, buffer + offset, 4 - parseread->bsize);
+ offset += 4 - parseread->bsize;
+ parseread->bsize = 0;
+ } else {
+ memcpy(tmp, buffer+offset, 4);
  offset += 4;
- break;
  }
+ tmp[4] = '\0';

- if (check == 0) {
- printf("Error, unable to parse object size\n");
- printf("Size bytes: %02x %02x %02x %02x\n",
- reply[offset+0],
- reply[offset+1],
- reply[offset+2],
- reply[offset+3]);
- printf("Data: %02x %02x %02x %02x\n",
- reply[offset+4],
- reply[offset+5],
- reply[offset+6],
- reply[offset+7]);
- exit(-1);
- }
- else {
- printf("No Error, object is %d bytes\n", parseread->osize);
- printf("Size bytes: %02x %02x %02x %02x\n",
- reply[offset+0],
- reply[offset+1],
- reply[offset+2],
- reply[offset+3]);
- printf("%02x %02x %02x %02x\n",
- reply[offset+4],
- reply[offset+5],
- reply[offset+6],
- reply[offset+7]);
+ printf("Size bytes: %02x %02x %02x %02x\n", tmp[0], tmp[1], tmp[2], tmp[3]);
+ parseread->osize = (int) strtol(tmp, NULL, 16);
+ printf("Parsed size: %d\n", parseread->osize);
+
+ if (parseread->osize == 0) {
+ printf("Only happens if reached terminating 0000\n");
+ break;
  }

- printf("Size: %d\n", parseread->osize);
- parseread->psize = 0;
+ parseread->psize = 4;

  // This classifies the object
- if ( strncmp(reply+offset+4, "NAK\n", 4)==0)
+ if ( strncmp(reply+offset, "NAK\n", 4)==0)
  parseread->state = STATE_NAK;
- else if (reply[offset+4] == 0x02)
+ else if (reply[offset] == 0x02)
  parseread->state = STATE_REMOTE;
  else
  parseread->state = STATE_UNKNOWN;
  }

- if ( (pool - offset) > (parseread->osize - parseread->psize) ) {
- printf("We have enough bytes, moving forward\n");
+ pending = parseread->osize - parseread->psize;
+ remaining = nmemb - offset;
+
+ if ( remaining > pending ) {
+ printf("Read complete; %d more bytes in buffer\n", remaining - pending);
  process_objects(reply+offset, parseread);
  offset += (parseread->osize - parseread->psize);
  parseread->state = STATE_NEWLINE;
  }
- else if ( (pool - offset) ==(parseread->osize - parseread->psize) ) {
- printf("They are the same length\n");
+ else if ( remaining == pending ) {
+ printf("Read complete; buffer exhausted\n");
  process_objects(reply+offset, parseread);
  offset += (parseread->osize - parseread->psize);
  parseread->state = STATE_NEWLINE;
  }
- else if ( (pool - offset) < (parseread->osize - parseread->psize)) {
- printf("We do not have enough bytes\n");
+ else if ( remaining < pending ) {
+ printf("Read complete; still missing %d bytes (%d read of %d)\n",
+ pending - remaining, parseread->psize + remaining, parseread->osize);
  process_objects(reply+offset, parseread);
- parseread->psize += (pool - offset);
- offset = pool;
+ parseread->psize += remaining;
+ offset = nmemb;
  }
  }

  printf("Offset: %d\n", offset);
+ if (offset < nmemb) {
+ // We don't have enough bytes to read the next size, so buffer it
+ printf("Short read; %d bytes left", nmemb - offset);
+ bzero(parseread->buffer, 4);
+ parseread->bsize = nmemb - offset;
+ memcpy(parseread->buffer, buffer+offset, parseread->bsize);
+
+ offset = nmemb;
+ }

  return offset;
 }
diff --git a/post.h b/post.h
index a229342..01a2669 100644
--- a/post.h
+++ b/post.h
@@ -5,6 +5,8 @@ struct parseread {
  int state; // current state
  int osize; // object size
  int psize; // processed size
+ char buffer[4];
+ int bsize;
 };

 #define STATE_NEWLINE 0

Again, it's not pretty, but with those changes a `./post` on your
openbsd URL succeeds.

========Start=========
Current stats: Current state: 999 Received: 503
Read complete; 77 more bytes in buffer
== New Object
Size bytes: 30 30 30 36
Parsed size: 6
Read complete; 71 more bytes in buffer
== New Object
Size bytes: 30 30 34 33
Parsed size: 67
Read complete; 4 more bytes in buffer
Remote: l 1798261 (delta 1438982), reused 1795494 (delta 1436215)
0000
== New Object
Size bytes: 30 30 30 30
Parsed size: 0
Only happens if reached terminating 0000
Offset: 503
curl_easy_perform() failed: No error

Bryan