Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

From: Johannes Sixt <j6t@kdbg.org>
Date: Jan 16, 2017, 20:33 UTC
Message-ID: <677a335f-889c-2924-b7bd-93c2b6663175@kdbg.org>
In-Reply-To: <alpine.DEB.2.20.1701161746200.3469@virtualbox>

Am 16.01.2017 um 18:06 schrieb Johannes Schindelin:

Show 20 quoted lines

> On Mon, 16 Jan 2017, Jeff King wrote:
>> Hmm.  I am not sure to what degree CRLFs are actually a problem here.
>> Keep in mind these are error messages generated via error(), and so not
>> processing arbitrary data. I can imagine that CRs might come from:
>
> Please note the regression test I added. It uses rev-parse's --abbrev-ref
> option which quotes the argument when erroring out. This argument then
> gets munged.
>
> So error() (or in this case, die()) *very much* processes arbitrary data.
>
> I *know* that rev-parse --abbrev-ref is an artificial example, it is
> highly unlikely that anybody will use
>
> 	git rev-parse --abbrev-ref "$(<call an external program here that
> 		generates CR/LF line endings>)"
>
> However, there are plenty other cases in regular Git usage where arguments
> are generated by external programs to which we have no business dictating a
> specific line ending style.

However, Jeff's patch is intended to catch exactly these cases (not for the cases where this happens accidentally, but when they happen with malicious intent).

We are talking about user-provided data that is reproduced by die() or error(). I daresay that we do not have a single case where it is intended that this data is intentionally multi-lined, like a commit message. It can only be an accident or malicious when it spans across lines.

I know we allow CR and LF in file names, but in all cases where such a name appears in an error message, it is *not important* that the data is reproduced exactly. On the contrary, it is usually more helpful to know that something strange is going on. The question marks are a strong indication to the user for this.

> If you absolutely insist, I will spend time to find a plausible example
> and use that in the regression test.

I don't want to see you on an endeavor with dubious results. I'd prefer to wait until the first case of "incorrectly munged data" is reported because, as I said, I have a gut feeling that there is none.

>> I am certainly open to loosening the sanitizing for CR to make things
>> work seamlessly on Windows. But I am having trouble imagining a case
>> that is actually negatively impacted.

I came to the same conclusion. I regret having sent out a warning message in, well, such a haste(*), without thinking the case through first. IMHO, Jeff's patch should be fine as is.

(*) literally; I had to catch a train.

-- Hannes