git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
To: Martin Ågren <martin.agren@gmail.com>
Cc: git@vger.kernel.org, Brandon Williams <bmwill@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	Stefan Beller <sbeller@google.com>
Subject: Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
Date: Mon, 04 Jun 2018 23:55:22 +0200
Message-ID: <87tvqiz06t.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20180604144305.29909-1-martin.agren@gmail.com>


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

> 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.

  parent reply index

Thread overview: 112+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-14 21:55 [PATCH 00/35] refactoring refspecs Brandon Williams
2018-05-14 21:55 ` [PATCH 01/35] refspec: move refspec parsing logic into its own file Brandon Williams
2018-05-15  8:06   ` Junio C Hamano
2018-05-15 16:51     ` Brandon Williams
2018-05-16  0:40       ` Junio C Hamano
2018-05-14 21:55 ` [PATCH 02/35] refspec: factor out parsing a single refspec Brandon Williams
2018-05-14 21:55 ` [PATCH 03/35] refspec: rename struct refspec to struct refspec_item Brandon Williams
2018-05-15  8:17   ` Junio C Hamano
2018-05-15 18:19     ` Brandon Williams
2018-05-14 21:55 ` [PATCH 04/35] refspec: introduce struct refspec Brandon Williams
2018-05-15  9:37   ` Junio C Hamano
2018-05-15 18:37     ` Brandon Williams
2018-05-14 21:55 ` [PATCH 05/35] refspec: convert valid_fetch_refspec to use parse_refspec Brandon Williams
2018-05-15  9:41   ` Junio C Hamano
2018-05-14 21:55 ` [PATCH 06/35] submodule--helper: convert push_check to use struct refspec Brandon Williams
2018-05-14 21:55 ` [PATCH 07/35] pull: convert get_tracking_branch to use refspec_item_init Brandon Williams
2018-05-14 21:55 ` [PATCH 08/35] transport: convert transport_push to use struct refspec Brandon Williams
2018-05-14 21:56 ` [PATCH 09/35] remote: convert check_push_refs " Brandon Williams
2018-05-14 21:56 ` [PATCH 10/35] remote: convert match_push_refs " Brandon Williams
2018-05-14 21:56 ` [PATCH 11/35] clone: convert cmd_clone to use refspec_item_init Brandon Williams
2018-05-14 21:56 ` [PATCH 12/35] fast-export: convert to use struct refspec Brandon Williams
2018-05-14 21:56 ` [PATCH 13/35] remote: convert push refspecs to " Brandon Williams
2018-05-14 21:56 ` [PATCH 14/35] remote: convert fetch " Brandon Williams
2018-05-15  8:31   ` Ævar Arnfjörð Bjarmason
2018-05-15 17:57     ` Brandon Williams
2018-05-14 21:56 ` [PATCH 15/35] transport-helper: convert to use " Brandon Williams
2018-05-14 21:56 ` [PATCH 16/35] fetch: convert fetch_one " Brandon Williams
2018-05-14 21:56 ` [PATCH 17/35] fetch: convert refmap " Brandon Williams
2018-05-14 21:56 ` [PATCH 18/35] refspec: remove the deprecated functions Brandon Williams
2018-05-14 21:56 ` [PATCH 19/35] fetch: convert do_fetch to take a struct refspec Brandon Williams
2018-05-14 21:56 ` [PATCH 20/35] fetch: convert get_ref_map " Brandon Williams
2018-05-14 21:56 ` [PATCH 21/35] fetch: convert prune_refs " Brandon Williams
2018-05-14 21:56 ` [PATCH 22/35] remote: convert get_stale_heads " Brandon Williams
2018-05-14 21:56 ` [PATCH 23/35] remote: convert apply_refspecs " Brandon Williams
2018-05-14 21:56 ` [PATCH 24/35] remote: convert query_refspecs " Brandon Williams
2018-05-14 21:56 ` [PATCH 25/35] remote: convert get_ref_match " Brandon Williams
2018-05-14 21:56 ` [PATCH 26/35] remote: convert match_explicit_refs " Brandon Williams
2018-05-14 21:56 ` [PATCH 27/35] push: check for errors earlier Brandon Williams
2018-05-14 21:56 ` [PATCH 28/35] push: convert to use struct refspec Brandon Williams
2018-05-14 21:56 ` [PATCH 29/35] transport: convert transport_push to take a " Brandon Williams
2018-05-14 21:56 ` [PATCH 30/35] send-pack: store refspecs in " Brandon Williams
2018-05-14 21:56 ` [PATCH 31/35] transport: remove transport_verify_remote_names Brandon Williams
2018-05-14 21:56 ` [PATCH 32/35] http-push: store refspecs in a struct refspec Brandon Williams
2018-05-14 21:56 ` [PATCH 33/35] remote: convert match_push_refs to take " Brandon Williams
2018-05-14 21:56 ` [PATCH 34/35] remote: convert check_push_refs " Brandon Williams
2018-05-14 21:56 ` [PATCH 35/35] submodule: convert push_unpushed_submodules " Brandon Williams
2018-05-15  8:11   ` Ævar Arnfjörð Bjarmason
2018-05-15 16:52     ` Stefan Beller
2018-05-15 16:59     ` Brandon Williams
2018-05-14 23:08 ` [PATCH 00/35] refactoring refspecs Stefan Beller
2018-05-15  8:05 ` Junio C Hamano
2018-05-15  8:39 ` Ævar Arnfjörð Bjarmason
2018-05-15 18:01   ` Brandon Williams
2018-05-16 22:57 ` [PATCH v2 00/36] " Brandon Williams
2018-05-16 22:57   ` [PATCH v2 01/36] refspec: move refspec parsing logic into its own file Brandon Williams
2018-05-16 22:57   ` [PATCH v2 02/36] refspec: rename struct refspec to struct refspec_item Brandon Williams
2018-05-16 22:57   ` [PATCH v2 03/36] refspec: factor out parsing a single refspec Brandon Williams
2018-05-16 22:57   ` [PATCH v2 04/36] refspec: introduce struct refspec Brandon Williams
2018-05-16 22:57   ` [PATCH v2 05/36] refspec: convert valid_fetch_refspec to use parse_refspec Brandon Williams
2018-06-03 17:13     ` Martin Ågren
2018-06-04 14:43       ` [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()` Martin Ågren
2018-06-04 17:36         ` Brandon Williams
2018-06-04 21:55         ` Ævar Arnfjörð Bjarmason [this message]
2018-06-05  5:10           ` Martin Ågren
2018-06-05 16:29           ` Brandon Williams
2018-06-05 19:54             ` [PATCH 0/3] refspec: refactor & fix free() behavior Ævar Arnfjörð Bjarmason
2018-06-05 19:58               ` Brandon Williams
2018-06-05 20:20                 ` Martin Ågren
2018-06-05 19:54             ` [PATCH 1/3] refspec: s/refspec_item_init/&_or_die/g Ævar Arnfjörð Bjarmason
2018-06-05 19:54             ` [PATCH 2/3] refspec: add back a refspec_item_init() function Ævar Arnfjörð Bjarmason
2018-06-05 19:54             ` [PATCH 3/3] refspec: initalize `refspec_item` in `valid_fetch_refspec()` Ævar Arnfjörð Bjarmason
2018-05-16 22:57   ` [PATCH v2 06/36] submodule--helper: convert push_check to use struct refspec Brandon Williams
2018-05-16 22:57   ` [PATCH v2 07/36] pull: convert get_tracking_branch to use refspec_item_init Brandon Williams
2018-05-16 22:57   ` [PATCH v2 08/36] transport: convert transport_push to use struct refspec Brandon Williams
2018-05-16 22:57   ` [PATCH v2 09/36] remote: convert check_push_refs " Brandon Williams
2018-05-16 22:57   ` [PATCH v2 10/36] remote: convert match_push_refs " Brandon Williams
2018-05-16 22:57   ` [PATCH v2 11/36] clone: convert cmd_clone to use refspec_item_init Brandon Williams
2018-05-16 22:57   ` [PATCH v2 12/36] fast-export: convert to use struct refspec Brandon Williams
2018-05-16 22:58   ` [PATCH v2 13/36] remote: convert push refspecs to " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 14/36] remote: convert fetch " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 15/36] remote: remove add_prune_tags_to_fetch_refspec Brandon Williams
2018-05-16 22:58   ` [PATCH v2 16/36] transport-helper: convert to use struct refspec Brandon Williams
2018-05-16 22:58   ` [PATCH v2 17/36] fetch: convert fetch_one " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 18/36] fetch: convert refmap " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 19/36] refspec: remove the deprecated functions Brandon Williams
2018-05-16 22:58   ` [PATCH v2 20/36] fetch: convert do_fetch to take a struct refspec Brandon Williams
2018-05-16 22:58   ` [PATCH v2 21/36] fetch: convert get_ref_map " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 22/36] fetch: convert prune_refs " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 23/36] remote: convert get_stale_heads " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 24/36] remote: convert apply_refspecs " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 25/36] remote: convert query_refspecs " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 26/36] remote: convert get_ref_match " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 27/36] remote: convert match_explicit_refs " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 28/36] push: check for errors earlier Brandon Williams
2018-05-16 22:58   ` [PATCH v2 29/36] push: convert to use struct refspec Brandon Williams
2018-05-16 22:58   ` [PATCH v2 30/36] transport: convert transport_push to take a " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 31/36] send-pack: store refspecs in " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 32/36] transport: remove transport_verify_remote_names Brandon Williams
2018-05-16 22:58   ` [PATCH v2 33/36] http-push: store refspecs in a struct refspec Brandon Williams
2018-05-16 22:58   ` [PATCH v2 34/36] remote: convert match_push_refs to take " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 35/36] remote: convert check_push_refs " Brandon Williams
2018-05-16 22:58   ` [PATCH v2 36/36] submodule: convert push_unpushed_submodules " Brandon Williams
2018-05-16 23:48   ` [PATCH 0/2] generating ref-prefixes for configured refspecs Brandon Williams
2018-05-16 23:48     ` [PATCH 1/2] refspec: consolidate ref-prefix generation logic Brandon Williams
2018-05-31  0:43       ` Jonathan Nieder
2018-05-31  1:07         ` Jonathan Nieder
2018-05-31  7:23       ` [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1 Jonathan Nieder
2018-05-31 15:44         ` Brandon Williams
2018-06-01  2:12         ` Junio C Hamano
2018-06-01  2:49           ` Jonathan Nieder
2018-05-16 23:48     ` [PATCH 2/2] fetch: generate ref-prefixes when using a configured refspec Brandon Williams
2018-05-17 21:32     ` [PATCH 0/2] generating ref-prefixes for configured refspecs Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87tvqiz06t.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox