Re: [PATCH 3/4] ref-filter: merge get_obj and get_object

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 10, 2018, 09:23 UTC
Message-ID: <nycvar.QRO.7.76.6.1807101115570.75@tvgsbejvaqbjf.bet>
In-Reply-To: <010201647e19c24c-fd353913-225a-4153-aa0f-6ddf41ae6354-000000@eu-west-1.amazonses.com>

Hi Olga,

On Mon, 9 Jul 2018, Olga Telezhnaya wrote:

Show 15 quoted lines

> diff --git a/ref-filter.c b/ref-filter.c
> index 27733ef013bed..f04169f0ea0e3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1437,20 +1419,24 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>  }
>  
>  static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> -		       int deref, struct object **obj, struct strbuf *err)
> +		      int deref, struct object **obj, struct strbuf *err)
>  {
>  	int eaten;
>  	int ret = 0;
>  	unsigned long size;
> -	void *buf = get_obj(oid, obj, &size, &eaten);

So previously, `eaten` has been assigned always... but...

Show 12 quoted lines

> +	enum object_type type;
> +	void *buf = read_object_file(oid, &type, &size);
>  	if (!buf)
>  		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>  				      oid_to_hex(oid), ref->refname);
> -	else if (!*obj)
> -		ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> -				      oid_to_hex(oid), ref->refname);
> -	else
> -		grab_values(ref->value, deref, *obj, buf, size);
> +	else {
> +		*obj = parse_object_buffer(oid, type, size, buf, &eaten);

... now it only gets assigned in case `buf` was non-`NULL`... yet...

Show 8 quoted lines

> +		if (!*obj)
> +			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> +					      oid_to_hex(oid), ref->refname);
> +		else
> +			grab_values(ref->value, deref, *obj, buf, size);
> +	}
>  	if (!eaten)
>		free(buf);

... here, we still act on `eaten`. This causes GCC to complain thusly:

``` 2018-07-10T04:59:38.6368270Z ref-filter.c:1477:6: error: variable 'eaten' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] 2018-07-10T04:59:38.6468620Z if (oi->info.contentp) { 2018-07-10T04:59:38.6568710Z ^~~~~~~~~~~~~~~~~ 2018-07-10T04:59:38.6669970Z ref-filter.c:1489:7: note: uninitialized use occurs here 2018-07-10T04:59:38.6774240Z if (!eaten) 2018-07-10T04:59:38.6874860Z ^~~~~ 2018-07-10T04:59:38.6976740Z ref-filter.c:1477:2: note: remove the 'if' if its condition is always true 2018-07-10T04:59:38.7072330Z if (oi->info.contentp) { 2018-07-10T04:59:38.7172760Z ^~~~~~~~~~~~~~~~~~~~~~~ 2018-07-10T04:59:38.7274040Z ref-filter.c:1466:11: note: initialize the variable 'eaten' to silence this warning 2018-07-10T04:59:38.7374670Z int eaten; 2018-07-10T04:59:38.7474870Z ^ 2018-07-10T04:59:38.7575690Z = 0 ```

(See https://mseng.visualstudio.com/VSOnline/_build/results?buildId=6640204&view=logs for details)

I think that GCC is correct, and at the same time, it isn't. Because it does not matter whether `eaten` is uninitialized here: if it is, then `buf` is NULL, and the `free(buf);` call does nothing in particular.

However, it *is* sloppy to have a conditional block of code that acts on an uninitialized value, whether it has adverse consequences or not. So I would suggest to initialize `eaten` to `1` (not `0` as suggested by GCC because we can avoid the no-op `free(buf)` while we're already touching that code path).

Ciao, Johannes