[v2,2/3] ls-refs.c: initialize 'prefixes' before using it

Commit Message

Taylor Blau Jan. 20, 2021, 4:04 p.m. UTC

From: Jacob Vosmaer <[e-mail]>

Correctly initialize the "prefixes" strvec using strvec_init() instead
of simply zeroing it via the earlier memset().

There's no way to trigger a crash, since the first 'ref-prefix' command
will initialize the strvec via the 'ALLOC_GROW' in 'strvec_push_nodup()'
(the alloc and nr variables are already zero'd, so the call to
ALLOC_GROW is valid).

If no "ref-prefix" command was given, then the call to
'ls-refs.c:ref_match()' will abort early after it reads the zero in
'prefixes->nr'. Likewise, strvec_clear() will only call free() on the
array, which is NULL, so we're safe there, too.

But, all of this is dangerous and requires more reasoning than it would
if we simply called 'strvec_init()', so do that.

Signed-off-by: Jacob Vosmaer <[e-mail]>
Signed-off-by: Taylor Blau <[e-mail]>
---
 ls-refs.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jeff King Jan. 20, 2021, 7:58 p.m. UTC | #1

On Wed, Jan 20, 2021 at 11:04:25AM -0500, Taylor Blau wrote:

> From: Jacob Vosmaer <[e-mail]>
> 
> Correctly initialize the "prefixes" strvec using strvec_init() instead
> of simply zeroing it via the earlier memset().
> 
> There's no way to trigger a crash, since the first 'ref-prefix' command
> will initialize the strvec via the 'ALLOC_GROW' in 'strvec_push_nodup()'
> (the alloc and nr variables are already zero'd, so the call to
> ALLOC_GROW is valid).

For people not familiar with strvec, I think there's a missing bit of
explanation between those two paragraphs: an initialized strvec does not
point to NULL, but to a dummy array with a single NULL value in it. That
explains why it might crash. :)

That nit (and the one I mentioned in the previous patch) aside, these
patches look good to me (and I am OK with or without the nits
addressed).

-Peff

Taylor Blau Jan. 20, 2021, 8:13 p.m. UTC | #2

On Wed, Jan 20, 2021 at 02:58:21PM -0500, Jeff King wrote:
> That nit (and the one I mentioned in the previous patch) aside, these
> patches look good to me (and I am OK with or without the nits
> addressed).

Thanks, as always, for your review :-).

Thanks,
Taylor

Jacob Vosmaer Jan. 20, 2021, 9:50 p.m. UTC | #3

As the person whose name is on the "From:" line, I approve. And thanks!

On Wed, Jan 20, 2021 at 5:04 PM Taylor Blau <[e-mail]> wrote:
>
> From: Jacob Vosmaer <[e-mail]>
>
> Correctly initialize the "prefixes" strvec using strvec_init() instead
> of simply zeroing it via the earlier memset().
>
> There's no way to trigger a crash, since the first 'ref-prefix' command
> will initialize the strvec via the 'ALLOC_GROW' in 'strvec_push_nodup()'
> (the alloc and nr variables are already zero'd, so the call to
> ALLOC_GROW is valid).
>
> If no "ref-prefix" command was given, then the call to
> 'ls-refs.c:ref_match()' will abort early after it reads the zero in
> 'prefixes->nr'. Likewise, strvec_clear() will only call free() on the
> array, which is NULL, so we're safe there, too.
>
> But, all of this is dangerous and requires more reasoning than it would
> if we simply called 'strvec_init()', so do that.
>
> Signed-off-by: Jacob Vosmaer <[e-mail]>
> Signed-off-by: Taylor Blau <[e-mail]>
> ---
>  ls-refs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ls-refs.c b/ls-refs.c
> index a1e0b473e4..367597d447 100644
> --- a/ls-refs.c
> +++ b/ls-refs.c
> @@ -90,6 +90,7 @@ int ls_refs(struct repository *r, struct strvec *keys,
>         struct ls_refs_data data;
>
>         memset(&data, 0, sizeof(data));
> +       strvec_init(&data.prefixes);
>
>         git_config(ls_refs_config, NULL);
>
> --
> 2.30.0.138.g6d7191ea01
>

Patch

diff --git a/ls-refs.c b/ls-refs.c
index a1e0b473e4..367597d447 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -90,6 +90,7 @@  int ls_refs(struct repository *r, struct strvec *keys,
 	struct ls_refs_data data;
 
 	memset(&data, 0, sizeof(data));
+	strvec_init(&data.prefixes);
 
 	git_config(ls_refs_config, NULL);