Re: [PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 1, 2018, 02:01 UTC
- Message-ID
- <xmqqzi0f9see.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180530170433.191495-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> 'item->dst' has not been assigned if '!rhs' is true. As the caller is allowed to pass in uninitialized > memory (we don't assume 'item' was zeroed out before calling), this fixes an access to > uninitialized memory.
Did I miss the other 4 patches that this might depend on it?
Show 13 quoted lines
> diff --git a/refspec.c b/refspec.c
> index c59a4ccf1e5..ea169dec0d3 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -108,7 +108,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
> * - empty is not allowed.
> * - otherwise it must be a valid looking ref.
> */
> - if (!item->dst) {
> + if (!rhs) {
> if (check_refname_format(item->src, flags))
> return 0;
> } else if (!*item->dst) {Perhaps a better fisx is to explicitly assign NULL to item->dst when we see there is no right-hand-side.
Aside from the "uninitialized" issue, the original if/else cascade around here makes a lot more sense than the updated version. If we do not leave item->dst uninitialized, the control (and the readers' understanding) can flow without having to carry the invariant "item->dst is set ONLY when rhs != NULL" throughout this codepath, in order to understand that this if/else cascade is asking: is pointer NULL? then do one thing, otherwise is pointee NUL? then do another thing, otherwise we have a non-empty string so do something on it.