| Message ID | 20200815002509.2467645-2-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/3] refspec: fix documentation referring to refspec_item | expand |
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.
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 --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;