Re: [PATCHv3] tag: add --points-at list option
- From
Jeff King <peff@peff.net>
- Date
- Feb 8, 2012, 15:44 UTC
- Message-ID
- <20120208154442.GB8773@sigill.intra.peff.net>
- In-Reply-To
- <1328682076-23380-2-git-send-email-tmgrennan@gmail.com>
On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
Show 8 quoted lines
> +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.
Show 6 quoted lines
> + 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