[PATCH 0/6] off-by-one errors in git-daemon

[PATCH 0/6] off-by-one errors in git-daemon

[Jeff King]

This series fixes two off-by-one errors in git-daemon noticed by Michael
(who then nerd-sniped me into fixing them). It also improves
git-daemon's verbose logging of extended attributes, and beefs up the
tests a bit.

Before anyone gets excited, no, these aren't security-interesting
errors. The only effect you could have is for git-daemon to reject your
request as nonsense. ;)

  [1/6]: t5570: use ls-remote instead of clone for interp tests
  [2/6]: t/lib-git-daemon: record daemon log
  [3/6]: daemon: fix off-by-one in logging extended attributes
  [4/6]: daemon: handle NULs in extended attribute string
  [5/6]: t/lib-git-daemon: add network-protocol helpers
  [6/6]: daemon: fix length computation in newline stripping

 daemon.c                | 15 ++++++---------
 t/lib-git-daemon.sh     | 37 ++++++++++++++++++++++++++++++++++---
 t/t5570-git-daemon.sh   | 37 +++++++++++++++++++++++++++++++------
 t/test-lib-functions.sh | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 18 deletions(-)

-Peff

[PATCH 1/6] t5570: use ls-remote instead of clone for interp tests

[Jeff King]

We don't actually care about the clone operation here; we
just want to know if we were able to actually contact the
remote repository. Using ls-remote does that more
efficiently, and without us having to worry about managing
the tmp.git directory.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5570-git-daemon.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 225a022e8a..f92ebc5cd5 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -167,23 +167,20 @@ test_expect_success 'access repo via interpolated hostname' '
 	git init --bare "$repo" &&
 	git push "$repo" HEAD &&
 	>"$repo"/git-daemon-export-ok &&
-	rm -rf tmp.git &&
 	GIT_OVERRIDE_VIRTUAL_HOST=localhost \
-		git clone --bare "$GIT_DAEMON_URL/interp.git" tmp.git &&
-	rm -rf tmp.git &&
+		git ls-remote "$GIT_DAEMON_URL/interp.git" &&
 	GIT_OVERRIDE_VIRTUAL_HOST=LOCALHOST \
-		git clone --bare "$GIT_DAEMON_URL/interp.git" tmp.git
+		git ls-remote "$GIT_DAEMON_URL/interp.git"
 '
 
 test_expect_success 'hostname cannot break out of directory' '
-	rm -rf tmp.git &&
 	repo="$GIT_DAEMON_DOCUMENT_ROOT_PATH/../escape.git" &&
 	git init --bare "$repo" &&
 	git push "$repo" HEAD &&
 	>"$repo"/git-daemon-export-ok &&
 	test_must_fail \
 		env GIT_OVERRIDE_VIRTUAL_HOST=.. \
-		git clone --bare "$GIT_DAEMON_URL/escape.git" tmp.git
+		git ls-remote "$GIT_DAEMON_URL/escape.git"
 '
 
 stop_git_daemon
-- 
2.16.1.273.gfdaa03aa74

[PATCH 2/6] t/lib-git-daemon: record daemon log

[Jeff King]

When we start git-daemon for our tests, we send its stderr
log stream to a named pipe. We synchronously read the first
line to make sure that the daemon started, and then dump the
rest to descriptor 4. This is handy for debugging test
output with "--verbose", but the tests themselves can't
access the log data.

Let's dump the log into a file, as well, so that future
tests can check the log. There are two subtleties worth
calling out here:

  - we replace "cat" with a subshell loop around "read" to
    ensure that there's no buffering (so that tests can be
    sure that after a request has been served, the matching
    log entries will have made it to the file)

  - we open the logfile for append, rather than just output.
    That makes it OK for tests to truncate the logfile
    without restarting the daemon (the OS will atomically
    seek to the end of the file when outputting each line).
    That allows tests to look at the log without worrying
    about pollution from earlier tests.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-git-daemon.sh | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 987d40680b..19f3ffdbb1 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -53,11 +53,19 @@ start_git_daemon() {
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		>&3 2>git_daemon_output &
 	GIT_DAEMON_PID=$!
+	>daemon.log
 	{
 		read line <&7
+		echo "$line"
 		echo >&4 "$line"
-		cat <&7 >&4 &
-	} 7<git_daemon_output &&
+		(
+			while read line <&7
+			do
+				echo "$line"
+				echo >&4 "$line"
+			done
+		) &
+	} 7<git_daemon_output >>"$TRASH_DIRECTORY/daemon.log" &&
 
 	# Check expected output
 	if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
-- 
2.16.1.273.gfdaa03aa74

Re: [PATCH 2/6] t/lib-git-daemon: record daemon log

[Lucas Werkmeister]

On 25.01.2018 01:55, Jeff King wrote:
> When we start git-daemon for our tests, we send its stderr
> log stream to a named pipe. We synchronously read the first
> line to make sure that the daemon started, and then dump the
> rest to descriptor 4. This is handy for debugging test
> output with "--verbose", but the tests themselves can't
> access the log data.
> 
> Let's dump the log into a file, as well, so that future
> tests can check the log. There are two subtleties worth
> calling out here:
> 
>   - we replace "cat" with a subshell loop around "read" to
>     ensure that there's no buffering (so that tests can be
>     sure that after a request has been served, the matching
>     log entries will have made it to the file)

POSIX specifies the -u option for that behavior, can’t you use that?
(GNU coreutils’ cat ignores it, since writing without delay is
apparently its default behavior already.)

> 
>   - we open the logfile for append, rather than just output.
>     That makes it OK for tests to truncate the logfile
>     without restarting the daemon (the OS will atomically
>     seek to the end of the file when outputting each line).
>     That allows tests to look at the log without worrying
>     about pollution from earlier tests.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/lib-git-daemon.sh | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index 987d40680b..19f3ffdbb1 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -53,11 +53,19 @@ start_git_daemon() {
>  		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
>  		>&3 2>git_daemon_output &
>  	GIT_DAEMON_PID=$!
> +	>daemon.log
>  	{
>  		read line <&7
> +		echo "$line"
>  		echo >&4 "$line"
> -		cat <&7 >&4 &
> -	} 7<git_daemon_output &&
> +		(
> +			while read line <&7
> +			do
> +				echo "$line"
> +				echo >&4 "$line"
> +			done
> +		) &
> +	} 7<git_daemon_output >>"$TRASH_DIRECTORY/daemon.log" &&
>  
>  	# Check expected output
>  	if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
> 

read without -r clobbers backslashes, and echo may interpret escape
sequences. To faithfully reproduce the output, it would be better to use
read -r and printf '%s\n' "$line", I think. (However, it looks like the
existing code already uses read+echo, so I guess you could also keep
that pattern in this change and then fix it in a later one.)

Re: [PATCH 2/6] t/lib-git-daemon: record daemon log

[Jeff King]

On Thu, Jan 25, 2018 at 12:56:47PM +0100, Lucas Werkmeister wrote:

> > Let's dump the log into a file, as well, so that future
> > tests can check the log. There are two subtleties worth
> > calling out here:
> > 
> >   - we replace "cat" with a subshell loop around "read" to
> >     ensure that there's no buffering (so that tests can be
> >     sure that after a request has been served, the matching
> >     log entries will have made it to the file)
> 
> POSIX specifies the -u option for that behavior, can’t you use that?
> (GNU coreutils’ cat ignores it, since writing without delay is
> apparently its default behavior already.)

Actually, this glosses over one other detail, which is that we'd also
need to replace "cat" with "tee" to keep output going to descriptor 4.
That's not strictly necessary (it's just for debugging output), so we
could drop that. But the shell loop seemed easy enough.

> >  	{
> >  		read line <&7
> > +		echo "$line"
> >  		echo >&4 "$line"
> > -		cat <&7 >&4 &
> > -	} 7<git_daemon_output &&
> > +		(
> > +			while read line <&7
> > +			do
> > +				echo "$line"
> > +				echo >&4 "$line"
> > +			done
> > +		) &
> > +	} 7<git_daemon_output >>"$TRASH_DIRECTORY/daemon.log" &&
> >  
> >  	# Check expected output
> >  	if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
> > 
> 
> read without -r clobbers backslashes, and echo may interpret escape
> sequences. To faithfully reproduce the output, it would be better to use
> read -r and printf '%s\n' "$line", I think. (However, it looks like the
> existing code already uses read+echo, so I guess you could also keep
> that pattern in this change and then fix it in a later one.)

Yeah. I doubt it matters much, since this is just inside our tests, and
we control the input. But it doesn't hurt to do it in the more robust
way. I'll re-roll this patch.

-Peff

[PATCH 3/6] daemon: fix off-by-one in logging extended attributes

[Jeff King]

If receive a request like:

  git-upload-pack /foo.git\0host=localhost

we mark the offset of the NUL byte as "len", and then log
the bytes after the NUL with a "%.*s" placeholder, using
"pktlen - len" as the length, and "line + len + 1" as the
start of the string.

This is off-by-one, since the start of the string skips past
the separating NUL byte, but the adjusted length includes
it. Fortunately this doesn't actually read past the end of
the buffer, since "%.*s" will stop when it hits a NUL. And
regardless of what is in the buffer, packet_read() will
always add an extra NUL terminator for safety.

As an aside, the git.git client sends an extra NUL after a
"host" field, too, so we'd generally hit that one first, not
the one added by packet_read(). You can see this in the test
output which reports 15 bytes, even though the string has
only 14 bytes of visible data. But the point is that even a
client sending unusual data could not get us to read past
the end of the buffer, so this is purely a cosmetic fix.

Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
This code actually goes away in the next patch, but I thought it was
worth dealing with individually, since it is a memory error (albeit a
harmless one).

 daemon.c              |  4 ++--
 t/t5570-git-daemon.sh | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/daemon.c b/daemon.c
index e37e343d0a..d78afc8e96 100644
--- a/daemon.c
+++ b/daemon.c
@@ -759,8 +759,8 @@ static int execute(void)
 	len = strlen(line);
 	if (pktlen != len)
 		loginfo("Extended attributes (%d bytes) exist <%.*s>",
-			(int) pktlen - len,
-			(int) pktlen - len, line + len + 1);
+			(int) pktlen - len - 1,
+			(int) pktlen - len - 1, line + len + 1);
 	if (len && line[len-1] == '\n') {
 		line[--len] = 0;
 		pktlen--;
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index f92ebc5cd5..359af3994a 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -183,5 +183,16 @@ test_expect_success 'hostname cannot break out of directory' '
 		git ls-remote "$GIT_DAEMON_URL/escape.git"
 '
 
+test_expect_success 'daemon log records hostnames' '
+	cat >expect <<-\EOF &&
+	Extended attributes (15 bytes) exist <host=localhost>
+	EOF
+	>daemon.log &&
+	GIT_OVERRIDE_VIRTUAL_HOST=localhost \
+		git ls-remote "$GIT_DAEMON_URL/interp.git" &&
+	grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
+	test_cmp expect actual
+'
+
 stop_git_daemon
 test_done
-- 
2.16.1.273.gfdaa03aa74

[PATCH 4/6] daemon: handle NULs in extended attribute string

[Jeff King]

If we receive a request with extended attributes after the
NUL, we try to write those attributes to the log. We do so
with a "%s" format specifier, which will only show
characters up to the first NUL.

That's enough for printing a "host=" specifier. But since
dfe422d04d (daemon: recognize hidden request arguments,
2017-10-16) we may have another NUL, followed by protocol
parameters, and those are not logged at all.

Let's cut out the attempt to show the whole string, and
instead log when we parse individual attributes. We could
leave the "extended attributes (%d bytes) exist" part of the
log, which in theory could alert us to attributes that fail
to parse. But anything we don't parse as a "host=" parameter
gets blindly added to the "protocol" attribute, so we'd see
it in that part of the log.

Signed-off-by: Jeff King <peff@peff.net>
---
 daemon.c              | 9 ++++-----
 t/t5570-git-daemon.sh | 8 +++++---
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/daemon.c b/daemon.c
index d78afc8e96..652f89b6e7 100644
--- a/daemon.c
+++ b/daemon.c
@@ -597,6 +597,7 @@ static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 		if (strncasecmp("host=", extra_args, 5) == 0) {
 			val = extra_args + 5;
 			vallen = strlen(val) + 1;
+			loginfo("Extended attribute \"host\": %s", val);
 			if (*val) {
 				/* Split <host>:<port> at colon. */
 				char *host;
@@ -647,9 +648,11 @@ static void parse_extra_args(struct hostinfo *hi, struct argv_array *env,
 		}
 	}
 
-	if (git_protocol.len > 0)
+	if (git_protocol.len > 0) {
+		loginfo("Extended attribute \"protocol\": %s", git_protocol.buf);
 		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
 				 git_protocol.buf);
+	}
 	strbuf_release(&git_protocol);
 }
 
@@ -757,10 +760,6 @@ static int execute(void)
 	alarm(0);
 
 	len = strlen(line);
-	if (pktlen != len)
-		loginfo("Extended attributes (%d bytes) exist <%.*s>",
-			(int) pktlen - len - 1,
-			(int) pktlen - len - 1, line + len + 1);
 	if (len && line[len-1] == '\n') {
 		line[--len] = 0;
 		pktlen--;
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 359af3994a..b556469db6 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -183,13 +183,15 @@ test_expect_success 'hostname cannot break out of directory' '
 		git ls-remote "$GIT_DAEMON_URL/escape.git"
 '
 
-test_expect_success 'daemon log records hostnames' '
+test_expect_success 'daemon log records all attributes' '
 	cat >expect <<-\EOF &&
-	Extended attributes (15 bytes) exist <host=localhost>
+	Extended attribute "host": localhost
+	Extended attribute "protocol": version=1
 	EOF
 	>daemon.log &&
 	GIT_OVERRIDE_VIRTUAL_HOST=localhost \
-		git ls-remote "$GIT_DAEMON_URL/interp.git" &&
+		git -c protocol.version=1 \
+			ls-remote "$GIT_DAEMON_URL/interp.git" &&
 	grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
 	test_cmp expect actual
 '
-- 
2.16.1.273.gfdaa03aa74

[PATCH 5/6] t/lib-git-daemon: add network-protocol helpers

[Jeff King]

All of our git-protocol tests rely on invoking the client
and having it make a request of a server. That gives a nice
real-world test of how the two behave together, but it
doesn't leave any room for testing how a server might react
to _other_ clients.

Let's add a few test helper functions which can be used to
manually conduct a git-protocol conversation with a remote
git-daemon:

  1. To connect to a remote git-daemon, we need something
     like "netcat". But not everybody will have netcat. And
     even if they do, the behavior with respect to
     half-duplex shutdowns is not portable (openbsd netcat
     has "-N", with others you must rely on "-q 1", which is
     racy).

     Here we provide a "fake_nc" that is capable of doing
     a client-side netcat, with sane half-duplex semantics.
     It relies on perl's IO::Socket::INET. That's been in
     the base distribution since 5.6.0, so it's probably
     available everywhere. But just to be on the safe side,
     we'll add a prereq.

  2. To help tests speak and read pktline, this patch adds
     packetize() and depacketize() functions.

I've put fake_nc() into lib-git-daemon.sh, since that's
really the only server where we'd need to use a network
socket.  Whereas the pktline helpers may be of more general
use, so I've added them to test-lib-functions.sh. Programs
like upload-pack speak pktline, but can talk directly over
stdio without a network socket.

Signed-off-by: Jeff King <peff@peff.net>
---
The sideband handling in depacketize() is not actually needed for this
series. But these are helpers I had written long ago for some
unpublished work, and they have come in handy a few times.

 t/lib-git-daemon.sh     | 25 ++++++++++++++++++++++++-
 t/test-lib-functions.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 19f3ffdbb1..de621c1243 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -32,7 +32,8 @@ LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}
 
 GIT_DAEMON_PID=
 GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
-GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT
+GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT
+GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT
 
 start_git_daemon() {
 	if test -n "$GIT_DAEMON_PID"
@@ -98,3 +99,25 @@ stop_git_daemon() {
 	GIT_DAEMON_PID=
 	rm -f git_daemon_output
 }
+
+# A stripped-down version of a netcat client, that connects to a "host:port"
+# given in $1, sends its stdin followed by EOF, then dumps the response (until
+# EOF) to stdout.
+fake_nc() {
+	if ! test_declared_prereq FAKENC
+	then
+		echo >&4 "fake_nc: need to declare FAKENC prerequisite"
+		return 127
+	fi
+	perl -Mstrict -MIO::Socket::INET -e '
+		my $s = IO::Socket::INET->new(shift)
+			or die "unable to open socket: $!";
+		print $s <STDIN>;
+		$s->shutdown(1);
+		print <$s>;
+	' "$@"
+}
+
+test_lazy_prereq FAKENC '
+	perl -MIO::Socket::INET -e "exit 0"
+'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1701fe2a06..a679b02a1c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1020,3 +1020,37 @@ nongit () {
 		"$@"
 	)
 }
+
+# convert stdin to pktline representation; note that empty input becomes an
+# empty packet, not a flush packet (for that you can just print 0000 yourself).
+packetize() {
+	cat >packetize.tmp &&
+	len=$(wc -c <packetize.tmp) &&
+	printf '%04x%s' "$(($len + 4))" &&
+	cat packetize.tmp &&
+	rm -f packetize.tmp
+}
+
+# Parse the input as a series of pktlines, writing the result to stdout.
+# Sideband markers are removed automatically, and the output is routed to
+# stderr if appropriate.
+#
+# NUL bytes are converted to "\\0" for ease of parsing with text tools.
+depacketize () {
+	perl -e '
+		while (read(STDIN, $len, 4) == 4) {
+			if ($len eq "0000") {
+				print "FLUSH\n";
+			} else {
+				read(STDIN, $buf, hex($len) - 4);
+				$buf =~ s/\0/\\0/g;
+				if ($buf =~ s/^[\x2\x3]//) {
+					print STDERR $buf;
+				} else {
+					$buf =~ s/^\x1//;
+					print $buf;
+				}
+			}
+		}
+	'
+}
-- 
2.16.1.273.gfdaa03aa74

[PATCH 6/6] daemon: fix length computation in newline stripping

[Jeff King]

When git-daemon gets a pktline request, we strip off any
trailing newline, replacing it with a NUL. Clients prior to
5ad312bede (in git v1.4.0) would send:

  git-upload-pack repo.git\n

and we need to strip it off to understand their request.
After 5ad312bede, we send the host attribute but no newline,
like:

  git-upload-pack repo.git\0host=example.com\0

Both of these are parsed correctly by git-daemon. But if
some client were to combine the two:

  git-upload-pack repo.git\n\0host=example.com\0

we don't parse it correctly. The problem is that we use the
"len" variable to record the position of the NUL separator,
but then decrement it when we strip the newline. So we start
with:

  git-upload-pack repo.git\n\0host=example.com\0
                             ^-- len

and end up with:

  git-upload-pack repo.git\0\0host=example.com\0
                           ^-- len

This is arguably correct, since "len" tells us the length of
the initial string, but we don't actually use it for that.
What we do use it for is finding the offset of the extended
attributes; they used to be at len+1, but are now at len+2.

We can solve that by just leaving "len" where it is. We
don't have to care about the length of the shortened string,
since we just treat it like a C string.

No version of Git ever produced such a string, but it seems
like the daemon code meant to handle this case (and it seems
like a reasonable thing for somebody to do in a 3rd-party
implementation).

Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
 daemon.c              |  6 ++----
 t/t5570-git-daemon.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 652f89b6e7..72dfeaf6e2 100644
--- a/daemon.c
+++ b/daemon.c
@@ -760,10 +760,8 @@ static int execute(void)
 	alarm(0);
 
 	len = strlen(line);
-	if (len && line[len-1] == '\n') {
-		line[--len] = 0;
-		pktlen--;
-	}
+	if (len && line[len-1] == '\n')
+		line[len-1] = 0;
 
 	/* parse additional args hidden behind a NUL byte */
 	if (len != pktlen)
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index b556469db6..755b05a8ae 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -196,5 +196,20 @@ test_expect_success 'daemon log records all attributes' '
 	test_cmp expect actual
 '
 
+test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
+	{
+		printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
+		printf "0000"
+	} >input &&
+	fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
+	depacketize <output >output.raw &&
+
+	# just pick out the value of master, which avoids any protocol
+	# particulars
+	perl -lne "print \$1 if m{^(\\S+) refs/heads/master}" <output.raw >actual &&
+	git -C "$repo" rev-parse master >expect &&
+	test_cmp expect actual
+'
+
 stop_git_daemon
 test_done
-- 
2.16.1.273.gfdaa03aa74

Re: [PATCH 6/6] daemon: fix length computation in newline stripping

[Eric Sunshine]

On Wed, Jan 24, 2018 at 7:58 PM, Jeff King <peff@peff.net> wrote:
> When git-daemon gets a pktline request, we strip off any
> trailing newline, replacing it with a NUL. Clients prior to
> 5ad312bede (in git v1.4.0) would send: [...]
>
> Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> @@ -196,5 +196,20 @@ test_expect_success 'daemon log records all attributes' '
> +test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
> +       {
> +               printf "git-upload-pack /interp.git\n\0host=localhost" | packetize

Do we care about the &&-chain here? (We'd notice if something went
wrong in 'packetize' even without &&-chain since 'input' would likely
end up with incorrect content, but still...)

> +               printf "0000"
> +       } >input &&
> +       fake_nc "$GIT_DAEMON_HOST_PORT" <input >output &&
> +       depacketize <output >output.raw &&
> +
> +       # just pick out the value of master, which avoids any protocol
> +       # particulars
> +       perl -lne "print \$1 if m{^(\\S+) refs/heads/master}" <output.raw >actual &&
> +       git -C "$repo" rev-parse master >expect &&
> +       test_cmp expect actual
> +'

Re: [PATCH 6/6] daemon: fix length computation in newline stripping

[Jeff King]

On Thu, Jan 25, 2018 at 04:38:20PM -0500, Eric Sunshine wrote:

> On Wed, Jan 24, 2018 at 7:58 PM, Jeff King <peff@peff.net> wrote:
> > When git-daemon gets a pktline request, we strip off any
> > trailing newline, replacing it with a NUL. Clients prior to
> > 5ad312bede (in git v1.4.0) would send: [...]
> >
> > Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> > @@ -196,5 +196,20 @@ test_expect_success 'daemon log records all attributes' '
> > +test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
> > +       {
> > +               printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
> 
> Do we care about the &&-chain here? (We'd notice if something went
> wrong in 'packetize' even without &&-chain since 'input' would likely
> end up with incorrect content, but still...)

Hmm, yeah. It almost certainly wouldn't matter, but I think it's a good
idea to keep up our &&-chains as a pro forma thing.

-Peff

Re: [PATCH 0/6] off-by-one errors in git-daemon

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This series fixes two off-by-one errors in git-daemon noticed by Michael
> (who then nerd-sniped me into fixing them). It also improves
> git-daemon's verbose logging of extended attributes, and beefs up the
> tests a bit.
>
> Before anyone gets excited, no, these aren't security-interesting
> errors. The only effect you could have is for git-daemon to reject your
> request as nonsense. ;)

Thanks.  All looked sensible.

Re: [PATCH 0/6] off-by-one errors in git-daemon

[Jeff King]

On Thu, Jan 25, 2018 at 10:46:51AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This series fixes two off-by-one errors in git-daemon noticed by Michael
> > (who then nerd-sniped me into fixing them). It also improves
> > git-daemon's verbose logging of extended attributes, and beefs up the
> > tests a bit.
> >
> > Before anyone gets excited, no, these aren't security-interesting
> > errors. The only effect you could have is for git-daemon to reject your
> > request as nonsense. ;)
> 
> Thanks.  All looked sensible.

Thanks. Do you mind replacing patch 2 with the update below while
queuing? It uses the more robust loop mentioned by Lucas, and clarifies
the commit message a bit.

There should be no changes necessary for the other patches on top.

-- >8 --
Subject: [PATCH v2] t/lib-git-daemon: record daemon log

When we start git-daemon for our tests, we send its stderr
log stream to a named pipe. We synchronously read the first
line to make sure that the daemon started, and then dump the
rest to descriptor 4. This is handy for debugging test
output with "--verbose", but the tests themselves can't
access the log data.

Let's dump the log into a file, as well, so that future
tests can check the log. There are a few subtleties worth
calling out here:

  - we'll continue to send output to descriptor 4 for
    viewing/debugging, which would imply swapping out "cat"
    for "tee". But we want to ensure that there's no
    buffering, and "tee" doesn't have a standard way to
    ask for that. So we'll use a shell loop around "read"
    and "printf" instead. That ensures that after a request
    has been served, the matching log entries will have made
    it to the file.

  - the existing first-line shell loop used read/echo. We'll
    switch to consistently using "read -r" and "printf" to
    relay data as faithfully as possible.

  - we open the logfile for append, rather than just output.
    That makes it OK for tests to truncate the logfile
    without restarting the daemon (the OS will atomically
    seek to the end of the file when outputting each line).
    That allows tests to look at the log without worrying
    about pollution from earlier tests.

Helped-by: Lucas Werkmeister <mail@lucaswerkmeister.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-git-daemon.sh | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 987d40680b..9612cccefb 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -53,11 +53,19 @@ start_git_daemon() {
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		>&3 2>git_daemon_output &
 	GIT_DAEMON_PID=$!
+	>daemon.log
 	{
-		read line <&7
-		echo >&4 "$line"
-		cat <&7 >&4 &
-	} 7<git_daemon_output &&
+		read -r line <&7
+		printf "%s\n" "$line"
+		printf >&4 "%s\n" "$line"
+		(
+			while read -r line <&7
+			do
+				printf "%s\n" "$line"
+				printf >&4 "%s\n" "$line"
+			done
+		) &
+	} 7<git_daemon_output >>"$TRASH_DIRECTORY/daemon.log" &&
 
 	# Check expected output
 	if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
-- 
2.16.1.273.g775ca5227b