From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 09 Dec 2025 23:52:59 GMT
Subject: Re: [PATCH 2/3] remote-curl: fix memory leak in show_http_message()
Message-ID: <aTi2W0f03kwf0ONx@nand.local>
In-Reply-To: <438223792264169082db8a1be5cb419b657bda26.1764160227.git.gitgitgadget@gmail.com>

On Wed, Nov 26, 2025 at 12:30:26PM +0000, Vaidas Pilkauskas via GitGitGadget wrote:
> diff --git a/remote-curl.c b/remote-curl.c
> index 5959461cd3..dd0680e5ae 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -371,6 +371,7 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
>  			     struct strbuf *msg)
>  {
>  	const char *p, *eol;
> +	struct strbuf msgbuf = STRBUF_INIT;
>
>  	/*
>  	 * We only show text/plain parts, as other types are likely
> @@ -378,19 +379,24 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
>  	 */
>  	if (strcmp(type->buf, "text/plain"))
>  		return -1;
> +
> +	strbuf_addbuf(&msgbuf, msg);

Hmm. Looking at the list of show_http_message() callers, it looks like
they all follow the pattern of constructing a strbuf "msg", passing it
to this function, and then calling die() with some user-friendly
message.

I agree that the patch here does address that leak, but I wonder if we
should do it in a way that doesn't involve copying the "msg" buffer. One
thing we could do is rename 'show_http_message()' to make it clear that
it's fatal and then free the re-encoded buffer ourselves (along with the
other buffers type and charset), perhaps like so (on top of the previous
patch in lieu of this one):

--- 8< ---
diff --git a/remote-curl.c b/remote-curl.c
index 5959461cd34..9d8359665ee 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -367,23 +367,25 @@ static void free_discovery(struct discovery *d)
 	}
 }

-static int show_http_message(struct strbuf *type, struct strbuf *charset,
-			     struct strbuf *msg)
+static void show_http_message_fatal(struct strbuf *type, struct strbuf *charset,
+				    struct strbuf *msg, const char *fmt, ...)
 {
 	const char *p, *eol;
+	va_list ap;
+	report_fn die_message_routine = get_die_message_routine();

 	/*
 	 * We only show text/plain parts, as other types are likely
 	 * to be ugly to look at on the user's terminal.
 	 */
 	if (strcmp(type->buf, "text/plain"))
-		return -1;
+		goto out;
 	if (charset->len)
 		strbuf_reencode(msg, charset->buf, get_log_output_encoding());

 	strbuf_trim(msg);
 	if (!msg->len)
-		return -1;
+		goto out;

 	p = msg->buf;
 	do {
@@ -391,7 +393,15 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
 		fprintf(stderr, "remote: %.*s\n", (int)(eol - p), p);
 		p = eol + 1;
 	} while(*eol);
-	return 0;
+
+out:
+	strbuf_release(type);
+	strbuf_release(charset);
+	strbuf_release(msg);
+
+	va_start(ap, fmt);
+	die_message_routine(fmt, ap);
+	va_end(ap);
 }

 static int get_protocol_http_header(enum protocol_version version,
@@ -518,25 +528,27 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	case HTTP_OK:
 		break;
 	case HTTP_MISSING_TARGET:
-		show_http_message(&type, &charset, &buffer);
-		die(_("repository '%s' not found"),
-		    transport_anonymize_url(url.buf));
+		show_http_message_fatal(&type, &charset, &buffer,
+					_("repository '%s' not found"),
+					transport_anonymize_url(url.buf));
--- >8 ---

(...and so on for the remaining cases).

Thanks,
Taylor