Re: [PATCH v7 07/12] test-http-server: pass Git requests to http-backend

From: Jeff King <peff@peff.net>
To: Matthew John Cheetham via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Derrick Stolee" <derrickstolee@github.com>,
	"Lessley Dennington" <lessleydennington@gmail.com>,
	"Matthew John Cheetham" <mjcheetham@outlook.com>,
	"M Hickford" <mirth.hickford@gmail.com>,
	"Jeff Hostetler" <git@jeffhostetler.com>,
	"Glen Choo" <chooglen@google.com>,
	"Victoria Dye" <vdye@github.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v7 07/12] test-http-server: pass Git requests to http-backend
Date: Thu, 26 Jan 2023 04:37:30 -0500	[thread overview]
Message-ID: <Y9JJ2moUulG8gTba@coredump.intra.peff.net> (raw)
In-Reply-To: <ca9c2787248688cd7d8e20043a6ed75d93654e35.1674252531.git.gitgitgadget@gmail.com>

On Fri, Jan 20, 2023 at 10:08:45PM +0000, Matthew John Cheetham via GitGitGadget wrote:

> +static int is_git_request(struct req *req)
> +{
> +	static regex_t *smart_http_regex;
> +	static int initialized;
> +
> +	if (!initialized) {
> +		smart_http_regex = xmalloc(sizeof(*smart_http_regex));
> +		/*
> +		 * This regular expression matches all dumb and smart HTTP
> +		 * requests that are currently in use, and defined in
> +		 * Documentation/gitprotocol-http.txt.
> +		 *
> +		 */
> +		if (regcomp(smart_http_regex, "^/(HEAD|info/refs|"
> +			    "objects/info/[^/]+|git-(upload|receive)-pack)$",
> +			    REG_EXTENDED)) {
> +			warning("could not compile smart HTTP regex");
> +			smart_http_regex = NULL;
> +		}
> +		initialized = 1;
> +	}
> +
> +	return smart_http_regex &&
> +		!regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0);
> +}

Assigning NULL to smart_http_regex leaks the earlier allocation. You
could free it, but I have to wonder why it is on the heap in the first
place. Yes, you check for NULL and return 0 if it failed to compile,
but...why would it? It's hard-coded. And if it does fail, wouldn't you
want to fail immediately and loudly, because it means all of the tests
are broken?

I.e., something like this is a bit simpler:

diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
index 14d170e640..8048ba1636 100644
--- a/t/helper/test-http-server.c
+++ b/t/helper/test-http-server.c
@@ -327,28 +327,25 @@ static enum worker_result req__read(struct req *req, int fd)
 
 static int is_git_request(struct req *req)
 {
-	static regex_t *smart_http_regex;
+	static regex_t smart_http_regex;
 	static int initialized;
 
 	if (!initialized) {
-		smart_http_regex = xmalloc(sizeof(*smart_http_regex));
 		/*
 		 * This regular expression matches all dumb and smart HTTP
 		 * requests that are currently in use, and defined in
 		 * Documentation/gitprotocol-http.txt.
 		 *
 		 */
-		if (regcomp(smart_http_regex, "^/(HEAD|info/refs|"
+		if (regcomp(&smart_http_regex, "^/(HEAD|info/refs|"
 			    "objects/info/[^/]+|git-(upload|receive)-pack)$",
 			    REG_EXTENDED)) {
-			warning("could not compile smart HTTP regex");
-			smart_http_regex = NULL;
+			die("could not compile smart HTTP regex");
 		}
 		initialized = 1;
 	}
 
-	return smart_http_regex &&
-		!regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0);
+	return !regexec(&smart_http_regex, req->uri_path.buf, 0, NULL, 0);
 }
 
 static enum worker_result do__git(struct req *req, const char *user)

> +start_http_server () {
> +	#
> +	# Launch our server into the background in repo_dir.
> +	#
> +	(
> +		cd "$REPO_DIR"
> +		test-http-server --verbose \
> +			--listen=127.0.0.1 \
> +			--port=$GIT_TEST_HTTP_PROTOCOL_PORT \
> +			--reuseaddr \
> +			--pid-file="$PID_FILE" \
> +			"$@" \
> +			2>"$SERVER_LOG" &
> +	)
> +	#
> +	# Give it a few seconds to get started.
> +	#
> +	for k in 0 1 2 3 4
> +	do
> +		if test -f "$PID_FILE"
> +		then
> +			return 0
> +		fi
> +		sleep 1
> +	done

Yuck. This makes the test take a long time to run, since it will almost
always "sleep 1" each time (and it looks like you bring the server up
and down in several tests). Worse, it's at risk of failing racily if it
ever takes more than 5 seconds to start up. That should be uncommon, I'd
think, but could happen on a heavily loaded system.

There's a race-less solution using fifos in lib-git-daemon.sh, where we
wait for the "ready to rumble" line. It's kind of horrific, but it does
work and is battle-tested.

-Peff
