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

From: Jeff King <peff@peff.net>
Date: Jun 8, 2021, 21:57 UTC
Message-ID: <YL/n1b641m+wuVHG@coredump.intra.peff.net>
In-Reply-To: <20210608204841.2793591-1-rybak.a.v@gmail.com>

On Tue, Jun 08, 2021 at 10:48:41PM +0200, Andrei Rybak wrote:

Show 19 quoted lines

> Existing checks for scissors characters using memcmp(3) never read past
> the end of the line, because all substrings we are interested in are two
> characters long, and the outer loop guarantees we have at least one
> character.  So at most we will look at the NUL.
> 
> However, this is too subtle and may lead to bugs in code which copies
> this behavior without realizing substring length requirement.  So use
> starts_with() instead, which will stop at NUL regardless of the length
> of the prefix.  Remove extra pair of parentheses while we are here.
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  mailinfo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> This patch was originally part of:
> https://public-inbox.org/git/20190401215334.18678-1-rybak.a.v@gmail.com/
> I've finally gotten around to sending it on its own.

Well, after seeing the date I don't feel too bad for not remembering it. :)

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.

-Peff