On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer 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. > > Helped-by: Pedro Martelletto <pedro@xxxxxxxxxx> > Signed-off-by: Fabian Stelzer <fs@xxxxxxxxxxxx> Should this also have a "Helped-by: Junio" since this code was heavily inspired by his suggestion[1]? [1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/ > --- > diff --git a/gpg-interface.c b/gpg-interface.c > @@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, > + const char *next; > + for (line = ssh_principals_out.buf; > + *line; > + line = next) { > + const char *end_of_text; > + > + next = end_of_text = strchrnul(line, '\n'); > + > + /* Did we find a LF, and did we have CR before it? */ > + if (*end_of_text && > + line < end_of_text && > + end_of_text[-1] == '\r') > + end_of_text--; > + > + /* Unless we hit NUL, skip over the LF we found */ > + if (*next) > + next++; > + > + /* Not all lines are data. Skip empty ones */ > + if (line == end_of_text) > + continue; > + > + /* We now know we have an non-empty line. Process it */ > + principal = xmemdupz(line, end_of_text - line); Considering that this code makes a copy of the line _anyhow_ which it assigns to `principal`, it still seems like it would be simpler and far easier to understand at-a-glance to instead take advantage of one of the existing string-splitting functions. For instance, something like this: struct strbuf **line, **to_free; line = to_free = strbuf_split(&ssh_principals_out, '\n'); for (; *line; line++) { strbuf_trim_trailing_newline(*line); if (!(*line)->len) continue; principal = (*line)->buf; keeping in mind that strbuf_trim_trailing_newline() takes care of CR/LF, and with appropriate cleanup at the end of the loop: strbuf_list_free(to_free); (and removal of `FREE_AND_NULL(principal)` which is no longer needed). Something similar can be done with string_list_split(), as well.
