{"thread":{"id":"66060","subject":"[PATCH] remote: plug memory leaks","startedAt":"2026-07-25T00:43:29Z","lastAt":"2026-07-25T17:06:14Z","messageCount":5,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"548933","messageId":"xmqqv7a33nm9.fsf@gitster.g","threadId":"66060","inReplyTo":null,"subject":"[PATCH] remote: plug memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-25T00:43:26Z","receivedAt":"2026-07-25T00:43:29Z","isPatch":true,"body":"The in-core data structure used to keep track of\n'url.<real>.{insteadOf,pushInsteadOf} = <alias>' settings is not\nproperly cleaned up when the process is done with it.\n\nFix the rewrites_release() function to free not just the 'struct\nrewrites' instance itself, but also allocated structures that are\npointed at by the 'struct rewrites' instance.  One of the embedded\nstructures holds a 'const char *' to point at a borrowed constant\nstring from a configuration callback.  Since the code does not\nmodify this string, stop copying the value (alias URL) before\nregistering it in 'struct rewrite', as nobody is freeing this\nmember, to avoid leaking the extra copy.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * These are not recently introduced leaks as far as I can tell, but\n   the new tests in en/submodule-insteadof-remote-match expose them.\n\n remote.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 368a43c1b2..e3d4b25040 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -309,8 +309,11 @@ static struct rewrite *make_rewrite(struct rewrites *r,\n \n static void rewrites_release(struct rewrites *r)\n {\n-\tfor (int i = 0; i < r->rewrite_nr; i++)\n+\tfor (int i = 0; i < r->rewrite_nr; i++) {\n \t\tfree((char *)r->rewrite[i]->base);\n+\t\tfree(r->rewrite[i]->instead_of);\n+\t\tfree(r->rewrite[i]);\n+\t}\n \tfree(r->rewrite);\n \tmemset(r, 0, sizeof(*r));\n }\n@@ -469,13 +472,13 @@ static int handle_config(const char *key, const char *value,\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\trewrite = make_rewrite(&remote_state->rewrites, name,\n \t\t\t\t\t       namelen);\n-\t\t\tadd_instead_of(rewrite, xstrdup(value));\n+\t\t\tadd_instead_of(rewrite, value);\n \t\t} else if (!strcmp(subkey, \"pushinsteadof\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\trewrite = make_rewrite(&remote_state->rewrites_push,\n \t\t\t\t\t       name, namelen);\n-\t\t\tadd_instead_of(rewrite, xstrdup(value));\n+\t\t\tadd_instead_of(rewrite, value);\n \t\t}\n \t}\n \n-- \n2.55.0-576-g1c3ad6b142\n\n"},{"id":"548934","messageId":"xmqqbjbv3k7i.fsf@gitster.g","threadId":"66060","inReplyTo":"xmqqv7a33nm9.fsf@gitster.g","subject":"Re: [PATCH] remote: plug memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-25T01:57:05Z","receivedAt":"2026-07-25T01:57:08Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The in-core data structure used to keep track of\n> 'url.<real>.{insteadOf,pushInsteadOf} = <alias>' settings is not\n> properly cleaned up when the process is done with it.\n>\n> Fix the rewrites_release() function to free not just the 'struct\n> rewrites' instance itself, but also allocated structures that are\n> pointed at by the 'struct rewrites' instance.  One of the embedded\n> structures holds a 'const char *' to point at a borrowed constant\n> string from a configuration callback.  Since the code does not\n> modify this string, stop copying the value (alias URL) before\n> registering it in 'struct rewrite', as nobody is freeing this\n> member, to avoid leaking the extra copy.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * These are not recently introduced leaks as far as I can tell, but\n>    the new tests in en/submodule-insteadof-remote-match expose them.\n\nIt is unfortunately rare to see all CI jobs pass, but today is one\nof those days ;-)\n\nWith this, and everything in 'seen' reported in the last edition of\nthe \"What's cooking\" report, excluding the\n'tn/packfile-uri-concurrency' topic, CI passes all jobs.\n\n  https://github.com/git/git/actions/runs/30137079882/\n\n'tn/packfile-uri-concurrency' was tentatively excluded from the\nabove as I made a random guess at who the culprit for the t5550\nfailure in\n\n  https://github.com/git/git/actions/runs/30130205851/job/89602846186\n\nfor the SHA-256 CI job was.  I have merged the topic back into\n'seen', and the resulting CI run for 'seen' is here:\n\n  https://github.com/git/git/actions/runs/30138777784/\n\nIt has not finished running, so we'll see how it goes.\n\nThanks.\n"},{"id":"548968","messageId":"xmqqpl0b12gj.fsf@gitster.g","threadId":"66060","inReplyTo":"xmqqv7a33nm9.fsf@gitster.g","subject":"[PATCH v2] remote: plug memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-25T16:03:24Z","receivedAt":"2026-07-25T16:03:27Z","isPatch":true,"body":"The in-core data structure used to keep track of\n'url.<real>.{insteadOf,pushInsteadOf} = <alias>' settings is not\nproperly cleaned up when the process is done with it.\n\n'struct rewrites' is embedded in 'remote_state' and serves as the\ntop level of the rewrite data.  This holds an array of a variable\nnumber of pointers to 'struct rewrite' allocated individually on the\nheap.  Each 'struct rewrite' holds a '.base' string and an array of\n'struct counted_string' called '.instead_of', which is allocated\ncontiguously on the heap.  Each 'struct counted_string' has a\npointer to a string allocated on the heap.\n\nAmid these pointers, rewrites_release() fails to free everything\nother than 'struct rewrite''s '.base' member and the 'struct rewrite'\ninstances themselves.\n\nFix rewrites_release() to also free the contiguous array storing\n'.instead_of', the string pointers within each '.instead_of' element,\nand each 'struct rewrite' instance individually allocated on the heap.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n* The initial iteration relied on the assumption that strings\n  borrowed from the configset subsystem will not go away, attempting\n  to plug the leak of 'instead_of[n].s' pointers without making\n  copies.  However, it turns out that all existing users other than\n  a select few make copies and do not rely on that assumption.  In\n  this version, I decided to simply follow suit, which might be\n  slightly inefficient but is vastly safer.\n---\n remote.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex a664cd166a..6c84adb36a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -304,8 +304,15 @@ static struct rewrite *make_rewrite(struct rewrites *r,\n \n static void rewrites_release(struct rewrites *r)\n {\n-\tfor (int i = 0; i < r->rewrite_nr; i++)\n-\t\tfree((char *)r->rewrite[i]->base);\n+\tfor (int i = 0; i < r->rewrite_nr; i++) {\n+\t\tstruct rewrite *rewrite = r->rewrite[i];\n+\n+\t\tfree((char *)rewrite->base);\n+\t\tfor (int j = 0; j < rewrite->instead_of_nr; j++)\n+\t\t\tfree((char *)rewrite->instead_of[j].s);\n+\t\tfree(rewrite->instead_of);\n+\t\tfree(rewrite);\n+\t}\n \tfree(r->rewrite);\n \tmemset(r, 0, sizeof(*r));\n }\n\nRange-diff:\n1:  19a305bd22 ! 1:  3bd8668117 remote: plug memory leaks\n    @@ Commit message\n         'url.<real>.{insteadOf,pushInsteadOf} = <alias>' settings is not\n         properly cleaned up when the process is done with it.\n     \n    -    Fix the rewrites_release() function to free not just the 'struct\n    -    rewrites' instance itself, but also allocated structures that are\n    -    pointed at by the 'struct rewrites' instance.  One of the embedded\n    -    structures holds a 'const char *' to point at a borrowed constant\n    -    string from a configuration callback.  Since the code does not\n    -    modify this string, stop copying the value (alias URL) before\n    -    registering it in 'struct rewrite', as nobody is freeing this\n    -    member, to avoid leaking the extra copy.\n    +    'struct rewrites' is embedded in 'remote_state' and serves as the\n    +    top level of the rewrite data.  This holds an array of a variable\n    +    number of pointers to 'struct rewrite' allocated individually on the\n    +    heap.  Each 'struct rewrite' holds a '.base' string and an array of\n    +    'struct counted_string' called '.instead_of', which is allocated\n    +    contiguously on the heap.  Each 'struct counted_string' has a\n    +    pointer to a string allocated on the heap.\n    +\n    +    Amid these pointers, rewrites_release() fails to free everything\n    +    other than 'struct rewrite''s '.base' member and the 'struct rewrite'\n    +    instances themselves.\n    +\n    +    Fix rewrites_release() to also free the contiguous array storing\n    +    '.instead_of', the string pointers within each '.instead_of' element,\n    +    and each 'struct rewrite' instance individually allocated on the heap.\n     \n         Signed-off-by: Junio C Hamano <gitster@pobox.com>\n    +    ---\n    +\n    +    * The initial iteration relied on the assumption that strings\n    +      borrowed from the configset subsystem will not go away, attempting\n    +      to plug the leak of 'instead_of[n].s' pointers without making\n    +      copies.  However, it turns out that all existing users other than\n    +      a select few make copies and do not rely on that assumption.  In\n    +      this version, I decided to simply follow suit, which might be\n    +      slightly inefficient but is vastly safer.\n     \n      ## remote.c ##\n     @@ remote.c: static struct rewrite *make_rewrite(struct rewrites *r,\n    @@ remote.c: static struct rewrite *make_rewrite(struct rewrites *r,\n      static void rewrites_release(struct rewrites *r)\n      {\n     -\tfor (int i = 0; i < r->rewrite_nr; i++)\n    +-\t\tfree((char *)r->rewrite[i]->base);\n     +\tfor (int i = 0; i < r->rewrite_nr; i++) {\n    - \t\tfree((char *)r->rewrite[i]->base);\n    -+\t\tfree(r->rewrite[i]->instead_of);\n    -+\t\tfree(r->rewrite[i]);\n    ++\t\tstruct rewrite *rewrite = r->rewrite[i];\n    ++\n    ++\t\tfree((char *)rewrite->base);\n    ++\t\tfor (int j = 0; j < rewrite->instead_of_nr; j++)\n    ++\t\t\tfree((char *)rewrite->instead_of[j].s);\n    ++\t\tfree(rewrite->instead_of);\n    ++\t\tfree(rewrite);\n     +\t}\n      \tfree(r->rewrite);\n      \tmemset(r, 0, sizeof(*r));\n      }\n    -@@ remote.c: static int handle_config(const char *key, const char *value,\n    - \t\t\t\treturn config_error_nonbool(key);\n    - \t\t\trewrite = make_rewrite(&remote_state->rewrites, name,\n    - \t\t\t\t\t       namelen);\n    --\t\t\tadd_instead_of(rewrite, xstrdup(value));\n    -+\t\t\tadd_instead_of(rewrite, value);\n    - \t\t} else if (!strcmp(subkey, \"pushinsteadof\")) {\n    - \t\t\tif (!value)\n    - \t\t\t\treturn config_error_nonbool(key);\n    - \t\t\trewrite = make_rewrite(&remote_state->rewrites_push,\n    - \t\t\t\t\t       name, namelen);\n    --\t\t\tadd_instead_of(rewrite, xstrdup(value));\n    -+\t\t\tadd_instead_of(rewrite, value);\n    - \t\t}\n    - \t}\n    - \n-- \n2.55.0-570-g266ec51bf1\n\n"},{"id":"548969","messageId":"20260725161819.GA2343104@coredump.intra.peff.net","threadId":"66060","inReplyTo":"xmqqpl0b12gj.fsf@gitster.g","subject":"Re: [PATCH v2] remote: plug memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-25T16:18:19Z","receivedAt":"2026-07-25T16:18:26Z","isPatch":true,"body":"On Sat, Jul 25, 2026 at 09:03:24AM -0700, Junio C Hamano wrote:\n\n> * The initial iteration relied on the assumption that strings\n>   borrowed from the configset subsystem will not go away, attempting\n>   to plug the leak of 'instead_of[n].s' pointers without making\n>   copies.  However, it turns out that all existing users other than\n>   a select few make copies and do not rely on that assumption.  In\n>   this version, I decided to simply follow suit, which might be\n>   slightly inefficient but is vastly safer.\n\nHeh, I was just in the middle of writing a message digging into this.\n\nWe should not rely on that assumption, because we sometimes discard the\nconfigset (e.g., when discovering the repo, or when writing a new config\noption). I couldn't come up with a case that fails, but I think it is\nmostly luck (or lack of imagination) that there is no code path that\ninvalidates the configset between when we read the remote config and\nwhen we actually use it.\n\nSo even though in something like:\n\n    git -c url.$PWD.insteadOf=$PWD clone $PWD dst\n\nwe end up with a state were the instead-of structs are broken, nobody is\nreading them at that point.\n\nSo I think this v2 is doing the right thing.\n\n-Peff\n"},{"id":"548974","messageId":"xmqq1pcryp6j.fsf@gitster.g","threadId":"66060","inReplyTo":"20260725161819.GA2343104@coredump.intra.peff.net","subject":"Re: [PATCH v2] remote: plug memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-25T17:06:12Z","receivedAt":"2026-07-25T17:06:14Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> We should not rely on that assumption, because we sometimes discard the\n> configset (e.g., when discovering the repo, or when writing a new config\n> option). I couldn't come up with a case that fails, but I think it is\n> mostly luck (or lack of imagination) that there is no code path that\n> invalidates the configset between when we read the remote config and\n> when we actually use it.\n>\n> So even though in something like:\n>\n>     git -c url.$PWD.insteadOf=$PWD clone $PWD dst\n>\n> we end up with a state were the instead-of structs are broken, nobody is\n> reading them at that point.\n>\n> So I think this v2 is doing the right thing.\n\nThanks.\n\nI had somebody else dig into the entire codebase and they claim that\nthere is only one existing (ab)user of the configuration API that\nassumes that the configset-held strings will stay forever, which is\nthe comment_line_string stuff that is stored from the configuration\ncallback without getting copied.  I do not necessarily believe it is\nthe only one, but this particular code indeed seems to rely on the\nassumption.  #leftoverbits perhaps.\n\n"}]}