From 02adf84ab8996cb80d7468f1d3c13be19f929294 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 24 Jan 2018 19:55:05 -0500 Subject: [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 Signed-off-by: Junio C Hamano --- 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 From 314a73d6585bf9caecac3243f7de2d7330e1ff9f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 25 Jan 2018 14:16:41 -0500 Subject: [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 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- 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&4 "%s\n" "$line" + ( + while read -r line <&7 + do + printf "%s\n" "$line" + printf >&4 "%s\n" "$line" + done + ) & + } 7>"$TRASH_DIRECTORY/daemon.log" && # Check expected output if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble" From 19136be3f874ac265195ef35a8c5ed6c417eaea2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 24 Jan 2018 19:56:07 -0500 Subject: [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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- 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 + 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 From 550fbcad1c464df9e32cab15a8c6a01f91b1629c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 24 Jan 2018 19:56:20 -0500 Subject: [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 Signed-off-by: Junio C Hamano --- 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 : 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 + 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 ' From 4414a150025765bdf83df81026270b0acbb8b376 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 24 Jan 2018 19:58:19 -0500 Subject: [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 Signed-off-by: Junio C Hamano --- 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 9612cccefb..edbea2d986 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 ; + $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 Date: Wed, 24 Jan 2018 19:58:54 -0500 Subject: [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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- 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" output && + depacketize output.raw && + + # just pick out the value of master, which avoids any protocol + # particulars + perl -lne "print \$1 if m{^(\\S+) refs/heads/master}" actual && + git -C "$repo" rev-parse master >expect && + test_cmp expect actual +' + stop_git_daemon test_done