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

Am 16.01.2017 um 18:06 schrieb Johannes Schindelin:

On Mon, 16 Jan 2017, Jeff King wrote:

quoted

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.

quoted

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