threads / patch / 47687

patch, 6 partsoff-by-one errors in git-daemon

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

## tl;dr

13 messages between Jan 25, 2018 and Jan 26, 2018. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Jeff King· Jan 25, 2018, 00:54 UTC · lore

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
Jeff King· Jan 25, 2018, 00:55 UTC · re: Jeff King · lore

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

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(-)
Show changes to t/t5570-git-daemon.sh +3 −6
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
Jeff King· Jan 25, 2018, 00:55 UTC · re: Jeff King · lore

[PATCH 2/6] 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 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(-)
Show changes to t/lib-git-daemon.sh +10 −2
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
Lucas Werkmeister· Jan 25, 2018, 11:56 UTC · re: Jeff King · lore

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

On 25.01.2018 01:55, Jeff King wrote:
Show 15 quoted lines
> 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.)

Show 40 quoted lines
> 
>   - 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.)

Jeff King· Jan 25, 2018, 19:08 UTC · re: Lucas Werkmeister · lore

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

On Thu, Jan 25, 2018 at 12:56:47PM +0100, Lucas Werkmeister wrote:
Show 12 quoted lines
> > 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.

Show 24 quoted lines
> >  	{
> >  		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
Jeff King· Jan 25, 2018, 00:56 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +13 −2

daemon.c, t/t5570-git-daemon.sh

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
Jeff King· Jan 25, 2018, 00:56 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +9 −8

daemon.c, t/t5570-git-daemon.sh

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
Jeff King· Jan 25, 2018, 00:58 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +58 −1

t/lib-git-daemon.sh, t/test-lib-functions.sh

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
Jeff King· Jan 25, 2018, 00:58 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +17 −4

daemon.c, t/t5570-git-daemon.sh

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
Eric Sunshine· Jan 25, 2018, 21:38 UTC · re: Jeff King · lore

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

On Wed, Jan 24, 2018 at 7:58 PM, Jeff King <peff@peff.net> wrote:
Show 12 quoted lines
> 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...)

Show 11 quoted lines
> +               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
> +'
Jeff King· Jan 26, 2018, 18:52 UTC · re: Eric Sunshine · lore

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

On Thu, Jan 25, 2018 at 04:38:20PM -0500, Eric Sunshine wrote:
Show 17 quoted lines
> 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
Junio C Hamano· Jan 25, 2018, 18:46 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> 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.
Jeff King· Jan 25, 2018, 19:16 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 25, 2018 at 10:46:51AM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> 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(-)
Show changes to t/lib-git-daemon.sh +12 −4
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

← back to recent threads