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
