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

From: Jeff King <peff@peff.net>
Date: Jan 25, 2018, 19:16 UTC
Message-ID: <20180125191641.GB26309@sigill.intra.peff.net>
In-Reply-To: <xmqqshatai90.fsf@gitster.mtv.corp.google.com>

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

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