Re: [PATCH v2 2/3] ref-filter: fix the API to correctly handle CRLF

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2020, 17:50 UTC
Message-ID: <xmqqk13sjdz8.fsf@gitster.c.googlers.com>
In-Reply-To: <aab1f45ba976d088a8c68573a21ed2458915d6a6.1583807093.git.gitgitgadget@gmail.com>

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> The ref-filter API does not correctly handle commit or tag messages that
> ...

(I won't repeat myself here; see 0/3)

Show 31 quoted lines

> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  ref-filter.c             | 19 +++++++++++++++++--
>  t/t3203-branch-output.sh | 26 +++++++++++++++++++++-----
>  t/t6300-for-each-ref.sh  |  5 +++++
>  t/t7004-tag.sh           |  7 +++++++
>  4 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 79bb5206783..537cc4de42c 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1050,10 +1050,18 @@ static char *copy_subject(const char *buf, unsigned long len)
>  {
>  	char *r = xmemdupz(buf, len);
>  	int i;
> +	struct strbuf sb = STRBUF_INIT;
>  
> -	for (i = 0; i < len; i++)
> +	strbuf_attach(&sb, r, len, len + 1);
> +	for (i = 0; i < sb.len; i++) {
>  		if (r[i] == '\n')
>  			r[i] = ' ';
> +		if (r[i] == '\r') {
> +			strbuf_remove(&sb, i, 1);
> +			i -= 1;
> +		}
> +	}
> +	strbuf_detach(&sb, NULL);
>  	return r;
>  }

So, the chosen solution is to only remove CR and do so only in the first paragraph. Even if the second and subsequent paragraphs use CRLF line endings, those CRs are retained. Also, a lone CR in the first paragraph that is not part of CRLF end-of-line marker is lost, but other control characters like "\a" are retained. That sounds like an almost "minimum" change but not quite.

The way strbuf is used in the implementation is a bit curious and risky. Currently we do not realloc to shrink a strbuf, but when we start doing so, this code would break because you are relying on the fact that just before calling strbuf_detach(), sb.buf happens to be still the same as r.

As the point of the function is "we want to return a copy of what is in buf[0..len] but the input is a (possibly multi-line) paragraph, and we want a single line 'title', so replace end-of-line with a SP", a minimal translation that is more faithful to the intended meaning of the function would be:

	static char *copy_subject(const char *buf, unsigned long len)
	{
		struct strbuf sb = STRBUF_INIT;

		for (i = 0; i < len; i++) {
                	if (buf[i] == '\r' &&
			    i + 1 < len && buf[i + 1] == '\n')
                            continue; /* ignore CR in CRLF */

			if (buf[i] == '\n')
				strbuf_addch(&sb, ' ');
			else
				strbuf_addch(&sb, buf[i]);
		}
                return strbuf_detach(&sb, NULL);
	}

perhaps? This retains CR in the middle if exists just like BEL in the middle of the line, and uses strbuf in a safe way, I think.

Show 7 quoted lines

> @@ -1184,15 +1192,22 @@ static void find_subpos(const char *buf,
>  		eol = strchrnul(buf, '\n');
>  		if (*eol)
>  			eol++;
> +		/*  protect against messages that might contain \r\n */
> +		if (*eol == '\r')
> +			eol++;

This is quite convoluted. You found a LF and then are hoping to see if the byte after LF is CR (i.e. you are looking for LFCR, not CRLF).

Show 9 quoted lines

>  		buf = eol;
>  	}
>  	*sublen = buf - *sub;
>  	/* drop trailing newline, if present */
>  	if (*sublen && (*sub)[*sublen - 1] == '\n')
>  		*sublen -= 1;
> +	/*  protect against commit messages that might contain \r\n */
> +	else if (*sublen && (*sub)[*sublen - 1] == '\r')
> +		*sublen -= 3; /* drop '\r\n\r' */

Yeek. To find CR-LF-CR-LF, you look for CR-LF-CR? You only checked that the previous byte is NOT LF (because you are in else-if, so the previous if must have failed) and you have at least one previous byte that is CR. What gives us OK that *sublen is sufficiently long that we can safely subtract 3 from it (we only checked that it is not 0; who says it is 3 or more in this code???) and the two bytes before the CR we are looking at here are CRLF???