Re: [PATCH] mailinfo: use starts_with() when checking scissors

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2021, 02:22 UTC
Message-ID: <xmqq35trn5f1.fsf@gitster.g>
In-Reply-To: <YL/n1b641m+wuVHG@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> I think this is an improvement in safety and readability, and worth
> applying. I'd also be fine going further and using skip_prefix(), which
> would let us drop the magic-number "2", though:
>
>   - as Junio said in that earlier thread, we hope people aren't
>     encouraged to add more versions of scissors here anyway).
>
>   - I am not 100% sure that the increment of "c" in the conditional
>     is not also relying on that "2", as well (i.e., it is incrementing
>     one, and then the for-loop increments one). There's a non-trivial
>     risk of introducing off-by-one errors or even more subtlety here. :)
>
> So I'm OK leaving that. Thanks for resurrecting this cleanup.

You are correct to point out that the increment of "c" relies on 2. We increment by one here, and let the loop control to increment another, to skip over these two bytes.

The posted patch _might_ make it easier to read, but I do not think it improves safety at all. At the point of doing memcmp(), we know that *c is not NUL and because we are dealing with NUL-terminated string, we know we can check c[1] (otherwise we wouldn't even be able to see if c is pointing at the end of string), so we check c[0] and c[1] against four variants of two-byte scissors patterns. The current code uses memcmp() of 2 bytes, which is perfectly safe under the condition, and starts_with() would also be equally safe.

If we were to teach new scissors sequence that is longer than two bytes, then starts_with() would start becoming safer, but that will not happen, so...