[PATCH v2] remote: plug memory leaks

The in-core data structure used to keep track of
'url.<real>.{insteadOf,pushInsteadOf} = <alias>' settings is not
properly cleaned up when the process is done with it.

'struct rewrites' is embedded in 'remote_state' and serves as the
top level of the rewrite data.  This holds an array of a variable
number of pointers to 'struct rewrite' allocated individually on the
heap.  Each 'struct rewrite' holds a '.base' string and an array of
'struct counted_string' called '.instead_of', which is allocated
contiguously on the heap.  Each 'struct counted_string' has a
pointer to a string allocated on the heap.

Amid these pointers, rewrites_release() fails to free everything
other than 'struct rewrite''s '.base' member and the 'struct rewrite'
instances themselves.

Fix rewrites_release() to also free the contiguous array storing
'.instead_of', the string pointers within each '.instead_of' element,
and each 'struct rewrite' instance individually allocated on the heap.

Signed-off-by: Junio C Hamano <[email protected]>
---

* The initial iteration relied on the assumption that strings
  borrowed from the configset subsystem will not go away, attempting
  to plug the leak of 'instead_of[n].s' pointers without making
  copies.  However, it turns out that all existing users other than
  a select few make copies and do not rely on that assumption.  In
  this version, I decided to simply follow suit, which might be
  slightly inefficient but is vastly safer.
---
 remote.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index a664cd166a..6c84adb36a 100644
--- a/remote.c
+++ b/remote.c
@@ -304,8 +304,15 @@ static struct rewrite *make_rewrite(struct rewrites *r,
 
 static void rewrites_release(struct rewrites *r)
 {
-	for (int i = 0; i < r->rewrite_nr; i++)
-		free((char *)r->rewrite[i]->base);
+	for (int i = 0; i < r->rewrite_nr; i++) {
+		struct rewrite *rewrite = r->rewrite[i];
+
+		free((char *)rewrite->base);
+		for (int j = 0; j < rewrite->instead_of_nr; j++)
+			free((char *)rewrite->instead_of[j].s);
+		free(rewrite->instead_of);
+		free(rewrite);
+	}
 	free(r->rewrite);
 	memset(r, 0, sizeof(*r));
 }

Range-diff:
1:  19a305bd22 ! 1:  3bd8668117 remote: plug memory leaks
    @@ Commit message
         'url.<real>.{insteadOf,pushInsteadOf} = <alias>' settings is not
         properly cleaned up when the process is done with it.
     
    -    Fix the rewrites_release() function to free not just the 'struct
    -    rewrites' instance itself, but also allocated structures that are
    -    pointed at by the 'struct rewrites' instance.  One of the embedded
    -    structures holds a 'const char *' to point at a borrowed constant
    -    string from a configuration callback.  Since the code does not
    -    modify this string, stop copying the value (alias URL) before
    -    registering it in 'struct rewrite', as nobody is freeing this
    -    member, to avoid leaking the extra copy.
    +    'struct rewrites' is embedded in 'remote_state' and serves as the
    +    top level of the rewrite data.  This holds an array of a variable
    +    number of pointers to 'struct rewrite' allocated individually on the
    +    heap.  Each 'struct rewrite' holds a '.base' string and an array of
    +    'struct counted_string' called '.instead_of', which is allocated
    +    contiguously on the heap.  Each 'struct counted_string' has a
    +    pointer to a string allocated on the heap.
    +
    +    Amid these pointers, rewrites_release() fails to free everything
    +    other than 'struct rewrite''s '.base' member and the 'struct rewrite'
    +    instances themselves.
    +
    +    Fix rewrites_release() to also free the contiguous array storing
    +    '.instead_of', the string pointers within each '.instead_of' element,
    +    and each 'struct rewrite' instance individually allocated on the heap.
     
         Signed-off-by: Junio C Hamano <[email protected]>
    +    ---
    +
    +    * The initial iteration relied on the assumption that strings
    +      borrowed from the configset subsystem will not go away, attempting
    +      to plug the leak of 'instead_of[n].s' pointers without making
    +      copies.  However, it turns out that all existing users other than
    +      a select few make copies and do not rely on that assumption.  In
    +      this version, I decided to simply follow suit, which might be
    +      slightly inefficient but is vastly safer.
     
      ## remote.c ##
     @@ remote.c: static struct rewrite *make_rewrite(struct rewrites *r,
    @@ remote.c: static struct rewrite *make_rewrite(struct rewrites *r,
      static void rewrites_release(struct rewrites *r)
      {
     -	for (int i = 0; i < r->rewrite_nr; i++)
    +-		free((char *)r->rewrite[i]->base);
     +	for (int i = 0; i < r->rewrite_nr; i++) {
    - 		free((char *)r->rewrite[i]->base);
    -+		free(r->rewrite[i]->instead_of);
    -+		free(r->rewrite[i]);
    ++		struct rewrite *rewrite = r->rewrite[i];
    ++
    ++		free((char *)rewrite->base);
    ++		for (int j = 0; j < rewrite->instead_of_nr; j++)
    ++			free((char *)rewrite->instead_of[j].s);
    ++		free(rewrite->instead_of);
    ++		free(rewrite);
     +	}
      	free(r->rewrite);
      	memset(r, 0, sizeof(*r));
      }
    -@@ remote.c: static int handle_config(const char *key, const char *value,
    - 				return config_error_nonbool(key);
    - 			rewrite = make_rewrite(&remote_state->rewrites, name,
    - 					       namelen);
    --			add_instead_of(rewrite, xstrdup(value));
    -+			add_instead_of(rewrite, value);
    - 		} else if (!strcmp(subkey, "pushinsteadof")) {
    - 			if (!value)
    - 				return config_error_nonbool(key);
    - 			rewrite = make_rewrite(&remote_state->rewrites_push,
    - 					       name, namelen);
    --			add_instead_of(rewrite, xstrdup(value));
    -+			add_instead_of(rewrite, value);
    - 		}
    - 	}
    - 
-- 
2.55.0-570-g266ec51bf1