Re: [PATCH] * remote.c (valid_fetch_refspec):...

Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:12

Brandon Casey [off-list ref] writes:

quoted

Are you sure all the codepaths that stuff refspec[].{src,dst} give
freeable pointer?

remote.c:parse_refspec_internal() always does. This function is the
producer of the refspec that is being passed to free_refspecs() in the two
places where the patch called it.

The code paths for each additionally use of free_refspecs would have to
check that it is safe. Perhaps a comment is in order.

If you don't think we're ready for free_refspecs we can still call free()
manually in the two places I called free_refspecs.

Thanks.

A generic helper like this is preferable, as long as (1) you made sure
existing callsites are safe, and (2) the helper is clearly commented
against misuse by future callsites.

I personally do not think it would be a bad idea to even make a rule that
refspec[].{src,dst} _must_ be freeable pointers.  After all, we may have
to deal with repositories with insane number of refs (not in millions but
certainly in thousands), but it would be insane to have a remote with
insane number of refspecs (even in hundreds).

[PATCH] remote.c: add a function for deleting a refspec array and use it (twice)

From: Brandon Casey <hidden>
Date: 2016-06-15 22:45:12

A number of call sites allocate memory for a refspec array, populate
its members with heap memory, and then free only the refspec pointer
while leaking the memory allocated for the member elements. Provide
a function for freeing the elements of a refspec array and the array
itself.

Caution to callers: code paths must be checked to ensure that the
refspec members "src" and "dst" can be passed to free.

Signed-off-by: Brandon Casey <redacted>
---
 remote.c |   29 +++++++++++++++++++++++++++--
 remote.h |    1 +
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 105668f..3ef09a4 100644
--- a/remote.c
+++ b/remote.c

@@ -449,6 +449,26 @@ static int verify_refname(char *name, int is_glob)
 	return result;
 }
 
+/*
+ * This function frees a refspec array.
+ * Warning: code paths should be checked to ensure that the src
+ *          and dst pointers are always freeable pointers as well
+ *          as the refspec pointer itself.
+ */
+void free_refspecs(struct refspec *refspec, int nr_refspec)
+{
+	int i;
+
+	if (!refspec)
+		return;
+
+	for (i = 0; i < nr_refspec; i++) {
+		free(refspec[i].src);
+		free(refspec[i].dst);
+	}
+	free(refspec);
+}
+
 static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)
 {
 	int i;

@@ -567,7 +587,12 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 
  invalid:
 	if (verify) {
-		free(rs);
+		/*
+		 * nr_refspec must be greater than zero and i must be valid
+		 * since it is only possible to reach this point from within
+		 * the for loop above.
+		 */
+		free_refspecs(rs, i+1);
 		return NULL;
 	}
 	die("Invalid refspec '%s'", refspec[i]);

@@ -579,7 +604,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
 	struct refspec *refspec;
 
 	refspec = parse_refspec_internal(1, fetch_refspec, 1, 1);
-	free(refspec);
+	free_refspecs(refspec, 1);
 	return !!refspec;
 }

diff --git a/remote.h b/remote.h
index 091b1d0..2601f6e 100644
--- a/remote.h
+++ b/remote.h

@@ -78,6 +78,7 @@ void ref_remove_duplicates(struct ref *ref_map);
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
 struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);
+void free_refspecs(struct refspec *refspec, int nr_refspec);
 
 int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 	       int nr_refspec, const char **refspec, int all);

-- 
1.6.0.21.g35a2e

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help