Re: [PATCH 36/67] remote-ext: simplify git pkt-line generation

[Junio C Hamano]

Jeff King  writes:

>  static void send_git_request(int stdin_fd, const char *serv, const char 
> *repo,
>   const char *vhost)
>  {
> - size_t bufferspace;
> - size_t wpos = 0;
> - char *buffer;
> + struct strbuf buffer = STRBUF_INIT;
>  
> - /*
> -  * Request needs 12 bytes extra if there is vhost ( \0host=\0) and
> -  * 6 bytes extra ( \0) if there is no vhost.
> -  */
> + /* Generate packet with a dummy size header */
> + strbuf_addf(, "%s %s%c", serv, repo, 0);
>   if (vhost)
> - bufferspace = strlen(serv) + strlen(repo) + strlen(vhost) + 12;
> - else
> - bufferspace = strlen(serv) + strlen(repo) + 6;
> + strbuf_addf(, "host=%s%c", vhost, 0);
>  
> - if (bufferspace > 0x)

> + /* Now go back and fill in the size */
> + if (buffer.len > 0x)
>   die("Request too large to send");
> + xsnprintf(buffer.buf, buffer.alloc, "%04x", (unsigned)buffer.len);

So we now write "something something\0host=something" into the buffer
and then try to overwrite the first four bytes?  Does this xsnprintf()
stop after writing the four hexadecimal, or does it clobber the first
byte of the payload (i.e. copy of serv[0]) by a NUL termination?

>  
> + if (write_in_full(stdin_fd, buffer.buf, buffer.len) < 0)
>   die_errno("Failed to send request");
>  
> + strbuf_release();
>  }
>  
>  static int run_child(const char *arg, const char *service)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 36/67] remote-ext: simplify git pkt-line generation

[Jeff King]

On Wed, Sep 16, 2015 at 01:18:03PM -0700, Junio C Hamano wrote:

> > +   /* Now go back and fill in the size */
> > +   if (buffer.len > 0x)
> > die("Request too large to send");
> > +   xsnprintf(buffer.buf, buffer.alloc, "%04x", (unsigned)buffer.len);
> 
> So we now write "something something\0host=something" into the buffer
> and then try to overwrite the first four bytes?  Does this xsnprintf()
> stop after writing the four hexadecimal, or does it clobber the first
> byte of the payload (i.e. copy of serv[0]) by a NUL termination?

Argh, you're right, this is totally broken. We obviously have zero test
coverage of this feature. :(

This strategy cannot work at all without a way to format without adding
the NUL (or hackily saving and restoring the overwritten character).
The pkt-line code does its own hex formatting to get around this.

In fact...I think this whole patch can basically be replaced with a call
to packet_write().

Like this:

-- >8 --
Subject: [PATCH] remote-ext: simplify git pkt-line generation

We format a pkt-line into a heap buffer, which requires
manual computation of the required size, and uses some bare
sprintf calls. We could use a strbuf instead, which would
take care of the computation for us. But it's even easier
still to use packet_write(). Besides handling the formatting
and writing for us, it fixes two things:

  1. Our manual max-size check used 0x, while technically
 LARGE_PACKET_MAX is slightly smaller than this.

  2. Our packet will now be output as part of
 GIT_TRACE_PACKET debugging.

Unfortunately packet_write() does not let us build up the
buffer progressively, so we do have to repeat ourselves a
little depending on the "vhost" setting, but the end result
is still far more readable than the original.

Since there were no tests covering this feature at all,
we'll add a few into t5802.

Signed-off-by: Jeff King 
---
 builtin/remote-ext.c  | 34 +-
 t/t5802-connect-helper.sh | 28 
 2 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 3b8c22c..e3cd25d 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "transport.h"
 #include "run-command.h"
+#include "pkt-line.h"
 
 /*
  * URL syntax:
@@ -142,36 +143,11 @@ static const char **parse_argv(const char *arg, const 
char *service)
 static void send_git_request(int stdin_fd, const char *serv, const char *repo,
const char *vhost)
 {
-   size_t bufferspace;
-   size_t wpos = 0;
-   char *buffer;
-
-   /*
-* Request needs 12 bytes extra if there is vhost ( \0host=\0) and
-* 6 bytes extra ( \0) if there is no vhost.
-*/
-   if (vhost)
-   bufferspace = strlen(serv) + strlen(repo) + strlen(vhost) + 12;
+   if (!vhost)
+   packet_write(stdin_fd, "%s %s%c", serv, repo, 0);
else
-   bufferspace = strlen(serv) + strlen(repo) + 6;
-
-   if (bufferspace > 0x)
-   die("Request too large to send");
-   buffer = xmalloc(bufferspace);
-
-   /* Make the packet. */
-   wpos = sprintf(buffer, "%04x%s %s%c", (unsigned)bufferspace,
-   serv, repo, 0);
-
-   /* Add vhost if any. */
-   if (vhost)
-   sprintf(buffer + wpos, "host=%s%c", vhost, 0);
-
-   /* Send the request */
-   if (write_in_full(stdin_fd, buffer, bufferspace) < 0)
-   die_errno("Failed to send request");
-
-   free(buffer);
+   packet_write(stdin_fd, "%s %s%chost=%s%c", serv, repo, 0,
+vhost, 0);
 }
 
 static int run_child(const char *arg, const char *service)
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index 878faf2..b7a7f9d 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -69,4 +69,32 @@ test_expect_success 'update backfilled tag without primary 
transfer' '
test_cmp expect actual
 '
 
+
+test_expect_success 'set up fake git-daemon' '
+   mkdir remote &&
+   git init --bare remote/one.git &&
+   mkdir remote/host &&
+   git init --bare remote/host/two.git &&
+   write_script fake-daemon <<-\EOF &&
+   git daemon --inetd \
+   --informative-errors \
+   --export-all \
+   --base-path="$TRASH_DIRECTORY/remote" \
+   --interpolated-path="$TRASH_DIRECTORY/remote/%H%D" \
+   "$TRASH_DIRECTORY/remote"
+   EOF
+   export TRASH_DIRECTORY &&
+   PATH=$TRASH_DIRECTORY:$PATH
+'
+
+test_expect_success 'ext command can connect to git daemon (no vhost)' '
+   rm -rf dst &&
+   git clone "ext::fake-daemon %G/one.git" dst
+'
+
+test_expect_success 'ext command can connect to git daemon (vhost)' '
+   rm -rf dst &&
+   git clone "ext::fake-daemon %G/two.git %Vhost" dst
+'

[PATCH 36/67] remote-ext: simplify git pkt-line generation

[Jeff King]

We format a pkt-line into a heap buffer, which requires
manual computation of the required size. We can just use a
strbuf instead, which handles this for us, and lets us drop
some bare sprintf calls.

Note that we _could_ also use a fixed-size buffer here, as
we are limited by 16-bit pkt-line limit. But we'd still have
to worry about the length computation in that case, and the
allocation overhead is not important here.

Signed-off-by: Jeff King 
---
 builtin/remote-ext.c | 32 +---
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 3b8c22c..1d991d9 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -142,36 +142,22 @@ static const char **parse_argv(const char *arg, const 
char *service)
 static void send_git_request(int stdin_fd, const char *serv, const char *repo,
const char *vhost)
 {
-   size_t bufferspace;
-   size_t wpos = 0;
-   char *buffer;
+   struct strbuf buffer = STRBUF_INIT;
 
-   /*
-* Request needs 12 bytes extra if there is vhost ( \0host=\0) and
-* 6 bytes extra ( \0) if there is no vhost.
-*/
+   /* Generate packet with a dummy size header */
+   strbuf_addf(, "%s %s%c", serv, repo, 0);
if (vhost)
-   bufferspace = strlen(serv) + strlen(repo) + strlen(vhost) + 12;
-   else
-   bufferspace = strlen(serv) + strlen(repo) + 6;
+   strbuf_addf(, "host=%s%c", vhost, 0);
 
-   if (bufferspace > 0x)
+   /* Now go back and fill in the size */
+   if (buffer.len > 0x)
die("Request too large to send");
-   buffer = xmalloc(bufferspace);
-
-   /* Make the packet. */
-   wpos = sprintf(buffer, "%04x%s %s%c", (unsigned)bufferspace,
-   serv, repo, 0);
-
-   /* Add vhost if any. */
-   if (vhost)
-   sprintf(buffer + wpos, "host=%s%c", vhost, 0);
+   xsnprintf(buffer.buf, buffer.alloc, "%04x", (unsigned)buffer.len);
 
-   /* Send the request */
-   if (write_in_full(stdin_fd, buffer, bufferspace) < 0)
+   if (write_in_full(stdin_fd, buffer.buf, buffer.len) < 0)
die_errno("Failed to send request");
 
-   free(buffer);
+   strbuf_release();
 }
 
 static int run_child(const char *arg, const char *service)
-- 
2.6.0.rc2.408.ga2926b9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html