Re: [PATCHv3] tag: add --points-at list option

[Jeff King <peff@peff.net>]

On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:

> +static const unsigned char *match_points_at(const unsigned char *sha1)
> +{
> +	int i;
> +	const unsigned char *tagged_sha1 = (unsigned char*)"";
> +	struct object *obj = parse_object(sha1);
> +
> +	if (obj && obj->type == OBJ_TAG)
> +		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;

This is not safe. A sha1 is not NUL-terminated, but is rather _always_
20 bytes. So when the object is not a tag, you do the hashcmp against
your single-byte string literal above, and we end up comparing whatever
garbage is in the data segment after the string literal.

What you want instead is the all-zeros sha1, like:

  const unsigned char null_sha1[20] = { 0 };

Though we provide a null_sha1 global already. So doing:

  const unsigned char *tagged_sha1 = null_sha1;

would be sufficient.

That being said, I don't know why you want to do both lookups in the
same loop of the points_at. If it's a lightweight tag and the tag
matches, you can get away with not parsing the object at all (although
to be fair, that is the minority case, so it is unlikely to matter).

Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.

> +	for (i = 0; i < points_at.nr; i++)
> +		if (!hashcmp(points_at.sha1[i], sha1))
> +			return sha1;
> +		else if (!hashcmp(points_at.sha1[i], tagged_sha1))
> +			return tagged_sha1;
> +	return NULL;

Why write your own linear search? sha1_array_lookup will do a binary
search for you.

Other than that, the patch looks OK to me.

-Peff