On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@xxxxxxxxxxxx> wrote: > >> We need to trim \r from the output of 'ssh-keygen -Y find-principals' on > >> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer > >> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms > >> this hypothesis. Signature verification passes with the fix. > >> --- > >> - trust_size = strcspn(line, "\n"); > >> + trust_size = strcspn(line, "\n"); /* truncate at LF */ > >> + if (trust_size && trust_size != strlen(line) && > >> + line[trust_size - 1] == '\r') > >> + trust_size--; /* the LF was part of CRLF at the end */ > > > > I may be misunderstanding, but isn't the strlen() unnecessary? > > > > if (trust_size && line[trust_size] && > > line[trust_size - 1] == '\r') > > trust_size--; > > That changes behaviour when "line" has more than one lines in it. > strcspn() finds the first LF, and the posted patch ignores CRLF not > at the end of line[]. Your variant feels more correct if the > objective is to find the end of the first line (regardless of the > choice of the end-of-line convention, either LF or CRLF) and omit > the line terminator. Okay, that makes sense if that's the intention of the patch. Perhaps the commit message should mention that `line` might contain multiple lines and that it's only interested in the very last LF (unless it's already obvious to everyone else, even though it wasn't to me). I think it can still be done without strlen(), but it gets uglier and less obvious[*], so strlen() is probably the way to go, and I presume this isn't a hot path, so no big reason to avoid strlen(). [*] Like this, for instance, which is safe because there must be at least one character after the '\n' since this is a NUL-terminated string: if (trust_size && line[trust_size] == '\n' line[true_size + 1] == '\0' && line[trust_size - 1] == '\r')