From: Junio C Hamano Date: Tue, 04 Jan 2022 01:19:41 GMT Subject: Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen Message-ID: In-Reply-To: Eric Sunshine writes: > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano wrote: >> Eric Sunshine writes: >> > On Mon, Jan 3, 2022 at 9:24 AM 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. >> >> --- >> >> - 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 do not think that is the case. strcspn(line, "\n") will stop at the first one, so unless it is guaranteed that "line" has only one line in it, the patch as posted is not correct. Your variant without strlen() feels more correct, as I said.