Re: [PATCH 2/3] remote-curl: fix memory leak in show_http_message()
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Dec 9, 2025, 23:52 UTC
- 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:
Show 18 quoted lines
> 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