{"thread":{"id":"47687","subject":"[PATCH 0/6] off-by-one errors in git-daemon","startedAt":"2018-01-25T00:54:54Z","lastAt":"2018-01-26T18:52:58Z","messageCount":13,"participants":["Jeff King","Lucas Werkmeister","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"337347","messageId":"20180125005447.GA26661@sigill.intra.peff.net","threadId":"47687","inReplyTo":null,"subject":"[PATCH 0/6] off-by-one errors in git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T00:54:48Z","receivedAt":"2018-01-25T00:54:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes two off-by-one errors in git-daemon noticed by Michael\n(who then nerd-sniped me into fixing them). It also improves\ngit-daemon's verbose logging of extended attributes, and beefs up the\ntests a bit.\n\nBefore anyone gets excited, no, these aren't security-interesting\nerrors. The only effect you could have is for git-daemon to reject your\nrequest as nonsense. ;)\n\n  [1/6]: t5570: use ls-remote instead of clone for interp tests\n  [2/6]: t/lib-git-daemon: record daemon log\n  [3/6]: daemon: fix off-by-one in logging extended attributes\n  [4/6]: daemon: handle NULs in extended attribute string\n  [5/6]: t/lib-git-daemon: add network-protocol helpers\n  [6/6]: daemon: fix length computation in newline stripping\n\n daemon.c                | 15 ++++++---------\n t/lib-git-daemon.sh     | 37 ++++++++++++++++++++++++++++++++++---\n t/t5570-git-daemon.sh   | 37 +++++++++++++++++++++++++++++++------\n t/test-lib-functions.sh | 34 ++++++++++++++++++++++++++++++++++\n 4 files changed, 105 insertions(+), 18 deletions(-)\n\n-Peff\n"},{"id":"337348","messageId":"20180125005504.GA26850@sigill.intra.peff.net","threadId":"47687","inReplyTo":"20180125005447.GA26661@sigill.intra.peff.net","subject":"[PATCH 1/6] t5570: use ls-remote instead of clone for interp tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T00:55:05Z","receivedAt":"2018-01-25T00:55:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't actually care about the clone operation here; we\njust want to know if we were able to actually contact the\nremote repository. Using ls-remote does that more\nefficiently, and without us having to worry about managing\nthe tmp.git directory.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5570-git-daemon.sh | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 225a022e8a..f92ebc5cd5 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -167,23 +167,20 @@ test_expect_success 'access repo via interpolated hostname' '\n \tgit init --bare \"$repo\" &&\n \tgit push \"$repo\" HEAD &&\n \t>\"$repo\"/git-daemon-export-ok &&\n-\trm -rf tmp.git &&\n \tGIT_OVERRIDE_VIRTUAL_HOST=localhost \\\n-\t\tgit clone --bare \"$GIT_DAEMON_URL/interp.git\" tmp.git &&\n-\trm -rf tmp.git &&\n+\t\tgit ls-remote \"$GIT_DAEMON_URL/interp.git\" &&\n \tGIT_OVERRIDE_VIRTUAL_HOST=LOCALHOST \\\n-\t\tgit clone --bare \"$GIT_DAEMON_URL/interp.git\" tmp.git\n+\t\tgit ls-remote \"$GIT_DAEMON_URL/interp.git\"\n '\n \n test_expect_success 'hostname cannot break out of directory' '\n-\trm -rf tmp.git &&\n \trepo=\"$GIT_DAEMON_DOCUMENT_ROOT_PATH/../escape.git\" &&\n \tgit init --bare \"$repo\" &&\n \tgit push \"$repo\" HEAD &&\n \t>\"$repo\"/git-daemon-export-ok &&\n \ttest_must_fail \\\n \t\tenv GIT_OVERRIDE_VIRTUAL_HOST=.. \\\n-\t\tgit clone --bare \"$GIT_DAEMON_URL/escape.git\" tmp.git\n+\t\tgit ls-remote \"$GIT_DAEMON_URL/escape.git\"\n '\n \n stop_git_daemon\n-- \n2.16.1.273.gfdaa03aa74\n\n"},{"id":"337349","messageId":"20180125005511.GB26850@sigill.intra.peff.net","threadId":"47687","inReplyTo":"20180125005447.GA26661@sigill.intra.peff.net","subject":"[PATCH 2/6] t/lib-git-daemon: record daemon log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T00:55:11Z","receivedAt":"2018-01-25T00:55:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we start git-daemon for our tests, we send its stderr\nlog stream to a named pipe. We synchronously read the first\nline to make sure that the daemon started, and then dump the\nrest to descriptor 4. This is handy for debugging test\noutput with \"--verbose\", but the tests themselves can't\naccess the log data.\n\nLet's dump the log into a file, as well, so that future\ntests can check the log. There are two subtleties worth\ncalling out here:\n\n  - we replace \"cat\" with a subshell loop around \"read\" to\n    ensure that there's no buffering (so that tests can be\n    sure that after a request has been served, the matching\n    log entries will have made it to the file)\n\n  - we open the logfile for append, rather than just output.\n    That makes it OK for tests to truncate the logfile\n    without restarting the daemon (the OS will atomically\n    seek to the end of the file when outputting each line).\n    That allows tests to look at the log without worrying\n    about pollution from earlier tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/lib-git-daemon.sh | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 987d40680b..19f3ffdbb1 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -53,11 +53,19 @@ start_git_daemon() {\n \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n \t\t>&3 2>git_daemon_output &\n \tGIT_DAEMON_PID=$!\n+\t>daemon.log\n \t{\n \t\tread line <&7\n+\t\techo \"$line\"\n \t\techo >&4 \"$line\"\n-\t\tcat <&7 >&4 &\n-\t} 7<git_daemon_output &&\n+\t\t(\n+\t\t\twhile read line <&7\n+\t\t\tdo\n+\t\t\t\techo \"$line\"\n+\t\t\t\techo >&4 \"$line\"\n+\t\t\tdone\n+\t\t) &\n+\t} 7<git_daemon_output >>\"$TRASH_DIRECTORY/daemon.log\" &&\n \n \t# Check expected output\n \tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n-- \n2.16.1.273.gfdaa03aa74\n\n"},{"id":"337350","messageId":"20180125005606.GC26850@sigill.intra.peff.net","threadId":"47687","inReplyTo":"20180125005447.GA26661@sigill.intra.peff.net","subject":"[PATCH 3/6] daemon: fix off-by-one in logging extended attributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T00:56:07Z","receivedAt":"2018-01-25T00:56:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If receive a request like:\n\n  git-upload-pack /foo.git\\0host=localhost\n\nwe mark the offset of the NUL byte as \"len\", and then log\nthe bytes after the NUL with a \"%.*s\" placeholder, using\n\"pktlen - len\" as the length, and \"line + len + 1\" as the\nstart of the string.\n\nThis is off-by-one, since the start of the string skips past\nthe separating NUL byte, but the adjusted length includes\nit. Fortunately this doesn't actually read past the end of\nthe buffer, since \"%.*s\" will stop when it hits a NUL. And\nregardless of what is in the buffer, packet_read() will\nalways add an extra NUL terminator for safety.\n\nAs an aside, the git.git client sends an extra NUL after a\n\"host\" field, too, so we'd generally hit that one first, not\nthe one added by packet_read(). You can see this in the test\noutput which reports 15 bytes, even though the string has\nonly 14 bytes of visible data. But the point is that even a\nclient sending unusual data could not get us to read past\nthe end of the buffer, so this is purely a cosmetic fix.\n\nReported-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis code actually goes away in the next patch, but I thought it was\nworth dealing with individually, since it is a memory error (albeit a\nharmless one).\n\n daemon.c              |  4 ++--\n t/t5570-git-daemon.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex e37e343d0a..d78afc8e96 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -759,8 +759,8 @@ static int execute(void)\n \tlen = strlen(line);\n \tif (pktlen != len)\n \t\tloginfo(\"Extended attributes (%d bytes) exist <%.*s>\",\n-\t\t\t(int) pktlen - len,\n-\t\t\t(int) pktlen - len, line + len + 1);\n+\t\t\t(int) pktlen - len - 1,\n+\t\t\t(int) pktlen - len - 1, line + len + 1);\n \tif (len && line[len-1] == '\\n') {\n \t\tline[--len] = 0;\n \t\tpktlen--;\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex f92ebc5cd5..359af3994a 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -183,5 +183,16 @@ test_expect_success 'hostname cannot break out of directory' '\n \t\tgit ls-remote \"$GIT_DAEMON_URL/escape.git\"\n '\n \n+test_expect_success 'daemon log records hostnames' '\n+\tcat >expect <<-\\EOF &&\n+\tExtended attributes (15 bytes) exist <host=localhost>\n+\tEOF\n+\t>daemon.log &&\n+\tGIT_OVERRIDE_VIRTUAL_HOST=localhost \\\n+\t\tgit ls-remote \"$GIT_DAEMON_URL/interp.git\" &&\n+\tgrep -i extended.attribute daemon.log | cut -d\" \" -f2- >actual &&\n+\ttest_cmp expect actual\n+'\n+\n stop_git_daemon\n test_done\n-- \n2.16.1.273.gfdaa03aa74\n\n"},{"id":"337351","messageId":"20180125005619.GD26850@sigill.intra.peff.net","threadId":"47687","inReplyTo":"20180125005447.GA26661@sigill.intra.peff.net","subject":"[PATCH 4/6] daemon: handle NULs in extended attribute string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T00:56:20Z","receivedAt":"2018-01-25T00:56:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we receive a request with extended attributes after the\nNUL, we try to write those attributes to the log. We do so\nwith a \"%s\" format specifier, which will only show\ncharacters up to the first NUL.\n\nThat's enough for printing a \"host=\" specifier. But since\ndfe422d04d (daemon: recognize hidden request arguments,\n2017-10-16) we may have another NUL, followed by protocol\nparameters, and those are not logged at all.\n\nLet's cut out the attempt to show the whole string, and\ninstead log when we parse individual attributes. We could\nleave the \"extended attributes (%d bytes) exist\" part of the\nlog, which in theory could alert us to attributes that fail\nto parse. But anything we don't parse as a \"host=\" parameter\ngets blindly added to the \"protocol\" attribute, so we'd see\nit in that part of the log.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n daemon.c              | 9 ++++-----\n t/t5570-git-daemon.sh | 8 +++++---\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d78afc8e96..652f89b6e7 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -597,6 +597,7 @@ static char *parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)\n \t\tif (strncasecmp(\"host=\", extra_args, 5) == 0) {\n \t\t\tval = extra_args + 5;\n \t\t\tvallen = strlen(val) + 1;\n+\t\t\tloginfo(\"Extended attribute \\\"host\\\": %s\", val);\n \t\t\tif (*val) {\n \t\t\t\t/* Split <host>:<port> at colon. */\n \t\t\t\tchar *host;\n@@ -647,9 +648,11 @@ static void parse_extra_args(struct hostinfo *hi, struct argv_array *env,\n \t\t}\n \t}\n \n-\tif (git_protocol.len > 0)\n+\tif (git_protocol.len > 0) {\n+\t\tloginfo(\"Extended attribute \\\"protocol\\\": %s\", git_protocol.buf);\n \t\targv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT \"=%s\",\n \t\t\t\t git_protocol.buf);\n+\t}\n \tstrbuf_release(&git_protocol);\n }\n \n@@ -757,10 +760,6 @@ static int execute(void)\n \talarm(0);\n \n \tlen = strlen(line);\n-\tif (pktlen != len)\n-\t\tloginfo(\"Extended attributes (%d bytes) exist <%.*s>\",\n-\t\t\t(int) pktlen - len - 1,\n-\t\t\t(int) pktlen - len - 1, line + len + 1);\n \tif (len && line[len-1] == '\\n') {\n \t\tline[--len] = 0;\n \t\tpktlen--;\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 359af3994a..b556469db6 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -183,13 +183,15 @@ test_expect_success 'hostname cannot break out of directory' '\n \t\tgit ls-remote \"$GIT_DAEMON_URL/escape.git\"\n '\n \n-test_expect_success 'daemon log records hostnames' '\n+test_expect_success 'daemon log records all attributes' '\n \tcat >expect <<-\\EOF &&\n-\tExtended attributes (15 bytes) exist <host=localhost>\n+\tExtended attribute \"host\": localhost\n+\tExtended attribute \"protocol\": version=1\n \tEOF\n \t>daemon.log &&\n \tGIT_OVERRIDE_VIRTUAL_HOST=localhost \\\n-\t\tgit ls-remote \"$GIT_DAEMON_URL/interp.git\" &&\n+\t\tgit -c protocol.version=1 \\\n+\t\t\tls-remote \"$GIT_DAEMON_URL/interp.git\" &&\n \tgrep -i extended.attribute daemon.log | cut -d\" \" -f2- >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.16.1.273.gfdaa03aa74\n\n"},{"id":"337352","messageId":"20180125005819.GE26850@sigill.intra.peff.net","threadId":"47687","inReplyTo":"20180125005447.GA26661@sigill.intra.peff.net","subject":"[PATCH 5/6] t/lib-git-daemon: add network-protocol helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T00:58:19Z","receivedAt":"2018-01-25T00:58:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of our git-protocol tests rely on invoking the client\nand having it make a request of a server. That gives a nice\nreal-world test of how the two behave together, but it\ndoesn't leave any room for testing how a server might react\nto _other_ clients.\n\nLet's add a few test helper functions which can be used to\nmanually conduct a git-protocol conversation with a remote\ngit-daemon:\n\n  1. To connect to a remote git-daemon, we need something\n     like \"netcat\". But not everybody will have netcat. And\n     even if they do, the behavior with respect to\n     half-duplex shutdowns is not portable (openbsd netcat\n     has \"-N\", with others you must rely on \"-q 1\", which is\n     racy).\n\n     Here we provide a \"fake_nc\" that is capable of doing\n     a client-side netcat, with sane half-duplex semantics.\n     It relies on perl's IO::Socket::INET. That's been in\n     the base distribution since 5.6.0, so it's probably\n     available everywhere. But just to be on the safe side,\n     we'll add a prereq.\n\n  2. To help tests speak and read pktline, this patch adds\n     packetize() and depacketize() functions.\n\nI've put fake_nc() into lib-git-daemon.sh, since that's\nreally the only server where we'd need to use a network\nsocket.  Whereas the pktline helpers may be of more general\nuse, so I've added them to test-lib-functions.sh. Programs\nlike upload-pack speak pktline, but can talk directly over\nstdio without a network socket.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe sideband handling in depacketize() is not actually needed for this\nseries. But these are helpers I had written long ago for some\nunpublished work, and they have come in handy a few times.\n\n t/lib-git-daemon.sh     | 25 ++++++++++++++++++++++++-\n t/test-lib-functions.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 19f3ffdbb1..de621c1243 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -32,7 +32,8 @@ LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}\n \n GIT_DAEMON_PID=\n GIT_DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n-GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT\n+GIT_DAEMON_HOST_PORT=127.0.0.1:$LIB_GIT_DAEMON_PORT\n+GIT_DAEMON_URL=git://$GIT_DAEMON_HOST_PORT\n \n start_git_daemon() {\n \tif test -n \"$GIT_DAEMON_PID\"\n@@ -98,3 +99,25 @@ stop_git_daemon() {\n \tGIT_DAEMON_PID=\n \trm -f git_daemon_output\n }\n+\n+# A stripped-down version of a netcat client, that connects to a \"host:port\"\n+# given in $1, sends its stdin followed by EOF, then dumps the response (until\n+# EOF) to stdout.\n+fake_nc() {\n+\tif ! test_declared_prereq FAKENC\n+\tthen\n+\t\techo >&4 \"fake_nc: need to declare FAKENC prerequisite\"\n+\t\treturn 127\n+\tfi\n+\tperl -Mstrict -MIO::Socket::INET -e '\n+\t\tmy $s = IO::Socket::INET->new(shift)\n+\t\t\tor die \"unable to open socket: $!\";\n+\t\tprint $s <STDIN>;\n+\t\t$s->shutdown(1);\n+\t\tprint <$s>;\n+\t' \"$@\"\n+}\n+\n+test_lazy_prereq FAKENC '\n+\tperl -MIO::Socket::INET -e \"exit 0\"\n+'\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1701fe2a06..a679b02a1c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1020,3 +1020,37 @@ nongit () {\n \t\t\"$@\"\n \t)\n }\n+\n+# convert stdin to pktline representation; note that empty input becomes an\n+# empty packet, not a flush packet (for that you can just print 0000 yourself).\n+packetize() {\n+\tcat >packetize.tmp &&\n+\tlen=$(wc -c <packetize.tmp) &&\n+\tprintf '%04x%s' \"$(($len + 4))\" &&\n+\tcat packetize.tmp &&\n+\trm -f packetize.tmp\n+}\n+\n+# Parse the input as a series of pktlines, writing the result to stdout.\n+# Sideband markers are removed automatically, and the output is routed to\n+# stderr if appropriate.\n+#\n+# NUL bytes are converted to \"\\\\0\" for ease of parsing with text tools.\n+depacketize () {\n+\tperl -e '\n+\t\twhile (read(STDIN, $len, 4) == 4) {\n+\t\t\tif ($len eq \"0000\") {\n+\t\t\t\tprint \"FLUSH\\n\";\n+\t\t\t} else {\n+\t\t\t\tread(STDIN, $buf, hex($len) - 4);\n+\t\t\t\t$buf =~ s/\\0/\\\\0/g;\n+\t\t\t\tif ($buf =~ s/^[\\x2\\x3]//) {\n+\t\t\t\t\tprint STDERR $buf;\n+\t\t\t\t} else {\n+\t\t\t\t\t$buf =~ s/^\\x1//;\n+\t\t\t\t\tprint $buf;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\t'\n+}\n-- \n2.16.1.273.gfdaa03aa74\n\n"},{"id":"337353","messageId":"20180125005854.GF26850@sigill.intra.peff.net","threadId":"47687","inReplyTo":"20180125005447.GA26661@sigill.intra.peff.net","subject":"[PATCH 6/6] daemon: fix length computation in newline stripping","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T00:58:54Z","receivedAt":"2018-01-25T00:59:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-daemon gets a pktline request, we strip off any\ntrailing newline, replacing it with a NUL. Clients prior to\n5ad312bede (in git v1.4.0) would send:\n\n  git-upload-pack repo.git\\n\n\nand we need to strip it off to understand their request.\nAfter 5ad312bede, we send the host attribute but no newline,\nlike:\n\n  git-upload-pack repo.git\\0host=example.com\\0\n\nBoth of these are parsed correctly by git-daemon. But if\nsome client were to combine the two:\n\n  git-upload-pack repo.git\\n\\0host=example.com\\0\n\nwe don't parse it correctly. The problem is that we use the\n\"len\" variable to record the position of the NUL separator,\nbut then decrement it when we strip the newline. So we start\nwith:\n\n  git-upload-pack repo.git\\n\\0host=example.com\\0\n                             ^-- len\n\nand end up with:\n\n  git-upload-pack repo.git\\0\\0host=example.com\\0\n                           ^-- len\n\nThis is arguably correct, since \"len\" tells us the length of\nthe initial string, but we don't actually use it for that.\nWhat we do use it for is finding the offset of the extended\nattributes; they used to be at len+1, but are now at len+2.\n\nWe can solve that by just leaving \"len\" where it is. We\ndon't have to care about the length of the shortened string,\nsince we just treat it like a C string.\n\nNo version of Git ever produced such a string, but it seems\nlike the daemon code meant to handle this case (and it seems\nlike a reasonable thing for somebody to do in a 3rd-party\nimplementation).\n\nReported-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n daemon.c              |  6 ++----\n t/t5570-git-daemon.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 652f89b6e7..72dfeaf6e2 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -760,10 +760,8 @@ static int execute(void)\n \talarm(0);\n \n \tlen = strlen(line);\n-\tif (len && line[len-1] == '\\n') {\n-\t\tline[--len] = 0;\n-\t\tpktlen--;\n-\t}\n+\tif (len && line[len-1] == '\\n')\n+\t\tline[len-1] = 0;\n \n \t/* parse additional args hidden behind a NUL byte */\n \tif (len != pktlen)\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex b556469db6..755b05a8ae 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -196,5 +196,20 @@ test_expect_success 'daemon log records all attributes' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n+\t{\n+\t\tprintf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n+\t\tprintf \"0000\"\n+\t} >input &&\n+\tfake_nc \"$GIT_DAEMON_HOST_PORT\" <input >output &&\n+\tdepacketize <output >output.raw &&\n+\n+\t# just pick out the value of master, which avoids any protocol\n+\t# particulars\n+\tperl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/master}\" <output.raw >actual &&\n+\tgit -C \"$repo\" rev-parse master >expect &&\n+\ttest_cmp expect actual\n+'\n+\n stop_git_daemon\n test_done\n-- \n2.16.1.273.gfdaa03aa74\n"},{"id":"337361","messageId":"38b41541-5758-d013-3d64-314eec7e31ed@lucaswerkmeister.de","threadId":"47687","inReplyTo":"20180125005511.GB26850@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] t/lib-git-daemon: record daemon log","fromName":"Lucas Werkmeister","fromEmail":"mail@lucaswerkmeister.de","sentAt":"2018-01-25T11:56:47Z","receivedAt":"2018-01-25T11:56:55Z","isPatch":true,"sender":{"key":"mail@lucaswerkmeister.de","avatar":"https://avatars.githubusercontent.com/u/2346599?v=4"},"body":"On 25.01.2018 01:55, Jeff King wrote:\n> When we start git-daemon for our tests, we send its stderr\n> log stream to a named pipe. We synchronously read the first\n> line to make sure that the daemon started, and then dump the\n> rest to descriptor 4. This is handy for debugging test\n> output with \"--verbose\", but the tests themselves can't\n> access the log data.\n> \n> Let's dump the log into a file, as well, so that future\n> tests can check the log. There are two subtleties worth\n> calling out here:\n> \n>   - we replace \"cat\" with a subshell loop around \"read\" to\n>     ensure that there's no buffering (so that tests can be\n>     sure that after a request has been served, the matching\n>     log entries will have made it to the file)\n\nPOSIX specifies the -u option for that behavior, can’t you use that?\n(GNU coreutils’ cat ignores it, since writing without delay is\napparently its default behavior already.)\n\n> \n>   - we open the logfile for append, rather than just output.\n>     That makes it OK for tests to truncate the logfile\n>     without restarting the daemon (the OS will atomically\n>     seek to the end of the file when outputting each line).\n>     That allows tests to look at the log without worrying\n>     about pollution from earlier tests.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/lib-git-daemon.sh | 12 ++++++++++--\n>  1 file changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\n> index 987d40680b..19f3ffdbb1 100644\n> --- a/t/lib-git-daemon.sh\n> +++ b/t/lib-git-daemon.sh\n> @@ -53,11 +53,19 @@ start_git_daemon() {\n>  \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n>  \t\t>&3 2>git_daemon_output &\n>  \tGIT_DAEMON_PID=$!\n> +\t>daemon.log\n>  \t{\n>  \t\tread line <&7\n> +\t\techo \"$line\"\n>  \t\techo >&4 \"$line\"\n> -\t\tcat <&7 >&4 &\n> -\t} 7<git_daemon_output &&\n> +\t\t(\n> +\t\t\twhile read line <&7\n> +\t\t\tdo\n> +\t\t\t\techo \"$line\"\n> +\t\t\t\techo >&4 \"$line\"\n> +\t\t\tdone\n> +\t\t) &\n> +\t} 7<git_daemon_output >>\"$TRASH_DIRECTORY/daemon.log\" &&\n>  \n>  \t# Check expected output\n>  \tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n> \n\nread without -r clobbers backslashes, and echo may interpret escape\nsequences. To faithfully reproduce the output, it would be better to use\nread -r and printf '%s\\n' \"$line\", I think. (However, it looks like the\nexisting code already uses read+echo, so I guess you could also keep\nthat pattern in this change and then fix it in a later one.)\n"},{"id":"337391","messageId":"xmqqshatai90.fsf@gitster.mtv.corp.google.com","threadId":"47687","inReplyTo":"20180125005447.GA26661@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] off-by-one errors in git-daemon","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-25T18:46:51Z","receivedAt":"2018-01-25T18:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This series fixes two off-by-one errors in git-daemon noticed by Michael\n> (who then nerd-sniped me into fixing them). It also improves\n> git-daemon's verbose logging of extended attributes, and beefs up the\n> tests a bit.\n>\n> Before anyone gets excited, no, these aren't security-interesting\n> errors. The only effect you could have is for git-daemon to reject your\n> request as nonsense. ;)\n\nThanks.  All looked sensible.\n"},{"id":"337396","messageId":"20180125190824.GA26309@sigill.intra.peff.net","threadId":"47687","inReplyTo":"38b41541-5758-d013-3d64-314eec7e31ed@lucaswerkmeister.de","subject":"Re: [PATCH 2/6] t/lib-git-daemon: record daemon log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T19:08:24Z","receivedAt":"2018-01-25T19:08:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 25, 2018 at 12:56:47PM +0100, Lucas Werkmeister wrote:\n\n> > Let's dump the log into a file, as well, so that future\n> > tests can check the log. There are two subtleties worth\n> > calling out here:\n> > \n> >   - we replace \"cat\" with a subshell loop around \"read\" to\n> >     ensure that there's no buffering (so that tests can be\n> >     sure that after a request has been served, the matching\n> >     log entries will have made it to the file)\n> \n> POSIX specifies the -u option for that behavior, can’t you use that?\n> (GNU coreutils’ cat ignores it, since writing without delay is\n> apparently its default behavior already.)\n\nActually, this glosses over one other detail, which is that we'd also\nneed to replace \"cat\" with \"tee\" to keep output going to descriptor 4.\nThat's not strictly necessary (it's just for debugging output), so we\ncould drop that. But the shell loop seemed easy enough.\n\n> >  \t{\n> >  \t\tread line <&7\n> > +\t\techo \"$line\"\n> >  \t\techo >&4 \"$line\"\n> > -\t\tcat <&7 >&4 &\n> > -\t} 7<git_daemon_output &&\n> > +\t\t(\n> > +\t\t\twhile read line <&7\n> > +\t\t\tdo\n> > +\t\t\t\techo \"$line\"\n> > +\t\t\t\techo >&4 \"$line\"\n> > +\t\t\tdone\n> > +\t\t) &\n> > +\t} 7<git_daemon_output >>\"$TRASH_DIRECTORY/daemon.log\" &&\n> >  \n> >  \t# Check expected output\n> >  \tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n> > \n> \n> read without -r clobbers backslashes, and echo may interpret escape\n> sequences. To faithfully reproduce the output, it would be better to use\n> read -r and printf '%s\\n' \"$line\", I think. (However, it looks like the\n> existing code already uses read+echo, so I guess you could also keep\n> that pattern in this change and then fix it in a later one.)\n\nYeah. I doubt it matters much, since this is just inside our tests, and\nwe control the input. But it doesn't hurt to do it in the more robust\nway. I'll re-roll this patch.\n\n-Peff\n"},{"id":"337397","messageId":"20180125191641.GB26309@sigill.intra.peff.net","threadId":"47687","inReplyTo":"xmqqshatai90.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] off-by-one errors in git-daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-25T19:16:41Z","receivedAt":"2018-01-25T19:16:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 25, 2018 at 10:46:51AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This series fixes two off-by-one errors in git-daemon noticed by Michael\n> > (who then nerd-sniped me into fixing them). It also improves\n> > git-daemon's verbose logging of extended attributes, and beefs up the\n> > tests a bit.\n> >\n> > Before anyone gets excited, no, these aren't security-interesting\n> > errors. The only effect you could have is for git-daemon to reject your\n> > request as nonsense. ;)\n> \n> Thanks.  All looked sensible.\n\nThanks. Do you mind replacing patch 2 with the update below while\nqueuing? It uses the more robust loop mentioned by Lucas, and clarifies\nthe commit message a bit.\n\nThere should be no changes necessary for the other patches on top.\n\n-- >8 --\nSubject: [PATCH v2] t/lib-git-daemon: record daemon log\n\nWhen we start git-daemon for our tests, we send its stderr\nlog stream to a named pipe. We synchronously read the first\nline to make sure that the daemon started, and then dump the\nrest to descriptor 4. This is handy for debugging test\noutput with \"--verbose\", but the tests themselves can't\naccess the log data.\n\nLet's dump the log into a file, as well, so that future\ntests can check the log. There are a few subtleties worth\ncalling out here:\n\n  - we'll continue to send output to descriptor 4 for\n    viewing/debugging, which would imply swapping out \"cat\"\n    for \"tee\". But we want to ensure that there's no\n    buffering, and \"tee\" doesn't have a standard way to\n    ask for that. So we'll use a shell loop around \"read\"\n    and \"printf\" instead. That ensures that after a request\n    has been served, the matching log entries will have made\n    it to the file.\n\n  - the existing first-line shell loop used read/echo. We'll\n    switch to consistently using \"read -r\" and \"printf\" to\n    relay data as faithfully as possible.\n\n  - we open the logfile for append, rather than just output.\n    That makes it OK for tests to truncate the logfile\n    without restarting the daemon (the OS will atomically\n    seek to the end of the file when outputting each line).\n    That allows tests to look at the log without worrying\n    about pollution from earlier tests.\n\nHelped-by: Lucas Werkmeister <mail@lucaswerkmeister.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/lib-git-daemon.sh | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 987d40680b..9612cccefb 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -53,11 +53,19 @@ start_git_daemon() {\n \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n \t\t>&3 2>git_daemon_output &\n \tGIT_DAEMON_PID=$!\n+\t>daemon.log\n \t{\n-\t\tread line <&7\n-\t\techo >&4 \"$line\"\n-\t\tcat <&7 >&4 &\n-\t} 7<git_daemon_output &&\n+\t\tread -r line <&7\n+\t\tprintf \"%s\\n\" \"$line\"\n+\t\tprintf >&4 \"%s\\n\" \"$line\"\n+\t\t(\n+\t\t\twhile read -r line <&7\n+\t\t\tdo\n+\t\t\t\tprintf \"%s\\n\" \"$line\"\n+\t\t\t\tprintf >&4 \"%s\\n\" \"$line\"\n+\t\t\tdone\n+\t\t) &\n+\t} 7<git_daemon_output >>\"$TRASH_DIRECTORY/daemon.log\" &&\n \n \t# Check expected output\n \tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n-- \n2.16.1.273.g775ca5227b\n\n"},{"id":"337409","messageId":"CAPig+cTR39UAbMmKnoi4fhU6EjQp=VGcd0J6=_rS3iD+aixJOg@mail.gmail.com","threadId":"47687","inReplyTo":"20180125005854.GF26850@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] daemon: fix length computation in newline stripping","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-25T21:38:20Z","receivedAt":"2018-01-25T21:38:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 24, 2018 at 7:58 PM, Jeff King <peff@peff.net> wrote:\n> When git-daemon gets a pktline request, we strip off any\n> trailing newline, replacing it with a NUL. Clients prior to\n> 5ad312bede (in git v1.4.0) would send: [...]\n>\n> Reported-by: Michael Haggerty <mhagger@alum.mit.edu>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> @@ -196,5 +196,20 @@ test_expect_success 'daemon log records all attributes' '\n> +test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n> +       {\n> +               printf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n\nDo we care about the &&-chain here? (We'd notice if something went\nwrong in 'packetize' even without &&-chain since 'input' would likely\nend up with incorrect content, but still...)\n\n> +               printf \"0000\"\n> +       } >input &&\n> +       fake_nc \"$GIT_DAEMON_HOST_PORT\" <input >output &&\n> +       depacketize <output >output.raw &&\n> +\n> +       # just pick out the value of master, which avoids any protocol\n> +       # particulars\n> +       perl -lne \"print \\$1 if m{^(\\\\S+) refs/heads/master}\" <output.raw >actual &&\n> +       git -C \"$repo\" rev-parse master >expect &&\n> +       test_cmp expect actual\n> +'\n"},{"id":"337510","messageId":"20180126185251.GI27618@sigill.intra.peff.net","threadId":"47687","inReplyTo":"CAPig+cTR39UAbMmKnoi4fhU6EjQp=VGcd0J6=_rS3iD+aixJOg@mail.gmail.com","subject":"Re: [PATCH 6/6] daemon: fix length computation in newline stripping","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-26T18:52:52Z","receivedAt":"2018-01-26T18:52:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 25, 2018 at 04:38:20PM -0500, Eric Sunshine wrote:\n\n> On Wed, Jan 24, 2018 at 7:58 PM, Jeff King <peff@peff.net> wrote:\n> > When git-daemon gets a pktline request, we strip off any\n> > trailing newline, replacing it with a NUL. Clients prior to\n> > 5ad312bede (in git v1.4.0) would send: [...]\n> >\n> > Reported-by: Michael Haggerty <mhagger@alum.mit.edu>\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> > @@ -196,5 +196,20 @@ test_expect_success 'daemon log records all attributes' '\n> > +test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '\n> > +       {\n> > +               printf \"git-upload-pack /interp.git\\n\\0host=localhost\" | packetize\n> \n> Do we care about the &&-chain here? (We'd notice if something went\n> wrong in 'packetize' even without &&-chain since 'input' would likely\n> end up with incorrect content, but still...)\n\nHmm, yeah. It almost certainly wouldn't matter, but I think it's a good\nidea to keep up our &&-chains as a pro forma thing.\n\n-Peff\n"}]}