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

[Jeff King <peff@peff.net>]

On Wed, Jun 09, 2021 at 11:22:10AM +0900, Junio C Hamano wrote:

> 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...

Right, I agree the current code is safe. The main value would be trying
to make the correctness of the code more apparent. Perhaps a comment
would be better there, like:

diff --git a/mailinfo.c b/mailinfo.c
index ccc6beb27e..25b606db28 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -705,12 +705,17 @@ static int is_scissors_line(const char *line)
 			perforation++;
 			continue;
 		}
+		/*
+		 * passing 2 bytes to memcmp is safe here; we have at least
+		 * one non-NUL character from the loop condition, plus a
+		 * terminating NUL
+		 */
 		if ((!memcmp(c, ">8", 2) || !memcmp(c, "8<", 2) ||
 		     !memcmp(c, ">%", 2) || !memcmp(c, "%<", 2))) {
 			in_perforation = 1;
 			perforation += 2;
 			scissors += 2;
-			c++;
+			c++; /* only 1 here to account for loop increment */
 			continue;
 		}
 		in_perforation = 0;

Though since this is code we'd not plan to modify, and presumably
anybody touching it would have to fully grok the loop anyway, it might
not be that important.

I dunno. I offer it as an alternative (and am happy to add a commit
message). But I'm fine with leaving it as-is, too.

-Peff