diff mbox series

[RFC,2/3] refspec: make sure stack refspec_item variables are zeroed

Message ID 20200815002509.2467645-2-jacob.e.keller@intel.com
State New, archived
Headers show
Series [RFC,1/3] refspec: fix documentation referring to refspec_item | expand

Commit Message

Keller, Jacob E Aug. 15, 2020, 12:25 a.m. UTC
From: Jacob Keller <jacob.keller@gmail.com>

A couple of functions that used struct refspec_item did not zero out the
structure memory. This can result in unexpected behavior, especially if
additional parameters are ever added to refspec_item in the future. Use
memset to ensure that unset structure members are zero.

It may make sense to convert most of these uses of struct refspec_item
to use either struct initializers or refspec_item_init_or_die. However,
other similar code uses memset. Converting all of these uses has been
left as a future exercise.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/remote.c | 1 +
 transport.c      | 1 +
 2 files changed, 2 insertions(+)

Comments

Junio C Hamano Aug. 17, 2020, 4:33 p.m. UTC | #1
Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> A couple of functions that used struct refspec_item did not zero out the
> structure memory. This can result in unexpected behavior, especially if
> additional parameters are ever added to refspec_item in the future. Use
> memset to ensure that unset structure members are zero.
>
> It may make sense to convert most of these uses of struct refspec_item
> to use either struct initializers or refspec_item_init_or_die. However,
> other similar code uses memset. Converting all of these uses has been
> left as a future exercise.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  builtin/remote.c | 1 +
>  transport.c      | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index c8240e9fcd58..542f56e3878b 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -478,6 +478,7 @@ static int get_head_names(const struct ref *remote_refs, struct ref_states *stat
>  	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
>  	struct refspec_item refspec;
>  
> +	memset(&refspec, 0, sizeof(refspec));
>  	refspec.force = 0;
>  	refspec.pattern = 1;
>  	refspec.src = refspec.dst = "refs/heads/*";

The original leaves .matching and .exact_sha1 bitfields
uninitialized.  As .pattern is set to true, .exact_sha1
that is not initialized does not make get_fetch_map()
misbehave, and .matching is not used there, so the code
currently happens to be OK, but futureproofing is always
good.

> diff --git a/transport.c b/transport.c
> index 2d4fd851dc0f..419be0b6ea4b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -443,6 +443,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
>  	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
>  		return;
>  
> +	memset(&rs, 0, sizeof(rs));
>  	rs.src = ref->name;
>  	rs.dst = NULL;

The original here passes the rs to remote_find_tracking() with only
its .src and .dst filled.  The function is to find, given a concrete
ref, where the refspec tells it to go on the other end of the
connection, so the code currently happend to be OK without all other
fields, but again, futureproofing is good.
Jacob Keller Aug. 17, 2020, 4:49 p.m. UTC | #2
On Mon, Aug 17, 2020 at 9:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
> > From: Jacob Keller <jacob.keller@gmail.com>
> >
> > A couple of functions that used struct refspec_item did not zero out the
> > structure memory. This can result in unexpected behavior, especially if
> > additional parameters are ever added to refspec_item in the future. Use
> > memset to ensure that unset structure members are zero.
> >
> > It may make sense to convert most of these uses of struct refspec_item
> > to use either struct initializers or refspec_item_init_or_die. However,
> > other similar code uses memset. Converting all of these uses has been
> > left as a future exercise.
> >
> > Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> > ---
> >  builtin/remote.c | 1 +
> >  transport.c      | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index c8240e9fcd58..542f56e3878b 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -478,6 +478,7 @@ static int get_head_names(const struct ref *remote_refs, struct ref_states *stat
> >       struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
> >       struct refspec_item refspec;
> >
> > +     memset(&refspec, 0, sizeof(refspec));
> >       refspec.force = 0;
> >       refspec.pattern = 1;
> >       refspec.src = refspec.dst = "refs/heads/*";
>
> The original leaves .matching and .exact_sha1 bitfields
> uninitialized.  As .pattern is set to true, .exact_sha1
> that is not initialized does not make get_fetch_map()
> misbehave, and .matching is not used there, so the code
> currently happens to be OK, but futureproofing is always
> good.
>

Right. Today the unused fields happen to never get checked. But by
being uninitialized we risk unexpected behavior in case either new
fields are added or in case those fields start being checked by the
code using the structure.

> > diff --git a/transport.c b/transport.c
> > index 2d4fd851dc0f..419be0b6ea4b 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -443,6 +443,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
> >       if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
> >               return;
> >
> > +     memset(&rs, 0, sizeof(rs));
> >       rs.src = ref->name;
> >       rs.dst = NULL;
>
> The original here passes the rs to remote_find_tracking() with only
> its .src and .dst filled.  The function is to find, given a concrete
> ref, where the refspec tells it to go on the other end of the
> connection, so the code currently happend to be OK without all other
> fields, but again, futureproofing is good.
diff mbox series

Patch

diff --git a/builtin/remote.c b/builtin/remote.c
index c8240e9fcd58..542f56e3878b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -478,6 +478,7 @@  static int get_head_names(const struct ref *remote_refs, struct ref_states *stat
 	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
 	struct refspec_item refspec;
 
+	memset(&refspec, 0, sizeof(refspec));
 	refspec.force = 0;
 	refspec.pattern = 1;
 	refspec.src = refspec.dst = "refs/heads/*";
diff --git a/transport.c b/transport.c
index 2d4fd851dc0f..419be0b6ea4b 100644
--- a/transport.c
+++ b/transport.c
@@ -443,6 +443,7 @@  void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
 		return;
 
+	memset(&rs, 0, sizeof(rs));
 	rs.src = ref->name;
 	rs.dst = NULL;