Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 4, 2018, 21:55 UTC
Message-ID: <87tvqiz06t.fsf@evledraar.gmail.com>
In-Reply-To: <20180604144305.29909-1-martin.agren@gmail.com>

On Mon, Jun 04 2018, Martin Ågren wrote:

Show 38 quoted lines

> We allocate a `struct refspec_item` on the stack without initializing
> it. In particular, its `dst` and `src` members will contain some random
> data from the stack. When we later call `refspec_item_clear()`, it will
> call `free()` on those pointers. So if the call to `parse_refspec()` did
> not assign to them, we will be freeing some random "pointers". This is
> undefined behavior.
>
> To the best of my understanding, this cannot currently be triggered by
> user-provided data. And for what it's worth, the test-suite does not
> trigger this with SANITIZE=address. It can be provoked by calling
> `valid_fetch_refspec(":*")`.
>
> Zero the struct, as is done in other users of `struct refspec_item`.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> I found some time to look into this. It does not seem to be a
> user-visible bug, so not particularly critical.
>
>  refspec.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/refspec.c b/refspec.c
> index ada7854f7a..7dd7e361e5 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -189,7 +189,10 @@ void refspec_clear(struct refspec *rs)
>  int valid_fetch_refspec(const char *fetch_refspec_str)
>  {
>  	struct refspec_item refspec;
> -	int ret = parse_refspec(&refspec, fetch_refspec_str, REFSPEC_FETCH);
> +	int ret;
> +
> +	memset(&refspec, 0, sizeof(refspec));
> +	ret = parse_refspec(&refspec, fetch_refspec_str, REFSPEC_FETCH);
>  	refspec_item_clear(&refspec);
>  	return ret;
>  }

I think this makes more sense instead of this fix:

diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae85..74a804f2e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1077,7 +1077,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_required_reference.nr || option_optional_reference.nr)
 		setup_reference();

-	refspec_item_init(&refspec, value.buf, REFSPEC_FETCH);
+	refspec_item_init_or_die(&refspec, value.buf, REFSPEC_FETCH);

 	strbuf_reset(&value);

diff --git a/builtin/pull.c b/builtin/pull.c
index 1f2ecf3a88..bb64631d98 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -684,7 +684,7 @@ static const char *get_tracking_branch(const char *remote, const char *refspec)
 	const char *spec_src;
 	const char *merge_branch;

-	refspec_item_init(&spec, refspec, REFSPEC_FETCH);
+	refspec_item_init_or_die(&spec, refspec, REFSPEC_FETCH);
 	spec_src = spec.src;
 	if (!*spec_src || !strcmp(spec_src, "HEAD"))
 		spec_src = "HEAD";
diff --git a/refspec.c b/refspec.c
index 78edc48ae8..8806df0fd2 100644
--- a/refspec.c
+++ b/refspec.c
@@ -124,11 +124,16 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
 	return 1;
 }

-void refspec_item_init(struct refspec_item *item, const char *refspec, int fetch)
+int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch)
 {
 	memset(item, 0, sizeof(*item));
+	int ret = parse_refspec(item, refspec, fetch);
+	return ret;
+}

-	if (!parse_refspec(item, refspec, fetch))
+void refspec_item_init_or_die(struct refspec_item *item, const char *refspec, int fetch)
+{
+	if (!refspec_item_init(item, refspec, fetch))
 		die("Invalid refspec '%s'", refspec);
 }

@@ -152,7 +157,7 @@ void refspec_append(struct refspec *rs, const char *refspec)
 {
 	struct refspec_item item;

-	refspec_item_init(&item, refspec, rs->fetch);
+	refspec_item_init_or_die(&item, refspec, rs->fetch);

 	ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc);
 	rs->items[rs->nr++] = item;
@@ -191,7 +196,7 @@ void refspec_clear(struct refspec *rs)
 int valid_fetch_refspec(const char *fetch_refspec_str)
 {
 	struct refspec_item refspec;
-	int ret = parse_refspec(&refspec, fetch_refspec_str, REFSPEC_FETCH);
+	int ret = refspec_item_init(&refspec, fetch_refspec_str, REFSPEC_FETCH);
 	refspec_item_clear(&refspec);
 	return ret;
 }
diff --git a/refspec.h b/refspec.h
index 3a9363887c..ed5d997f7f 100644
--- a/refspec.h
+++ b/refspec.h
@@ -32,7 +32,8 @@ struct refspec {
 	int fetch;
 };

-void refspec_item_init(struct refspec_item *item, const char *refspec, int fetch);
+int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch);
+void refspec_item_init_or_die(struct refspec_item *item, const char *refspec, int fetch);
 void refspec_item_clear(struct refspec_item *item);
 void refspec_init(struct refspec *rs, int fetch);
 void refspec_append(struct refspec *rs, const char *refspec);

I.e. let's fix the bug, but with this admittedly more verbose fix we're
left with exactly two memset() in refspec.c, one for each type of struct
that's initialized by the API.

The reason this is difficult now is because the current API conflates
the init function with an init_or_die, which is what most callers want,
so let's just split those concerns up. Then we're left with one init
function that does the memset.