Re: [PATCH 1/1] rebase -r: let `label` generate safer labels

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

On 2019-09-02 at 18:29:56, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > Being picking I'll point out that ':' is not a valid in refs
> > either. Looking at
> > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I
> > think only " and | are not allowed on NTFS/FAT but are valid in refs
> > (see the man page for git check-ref-format for all the details). So
> > the main limitation is actually what git allows in refs.
> 
> Yeah, trying to use the contents of the log message without
> sufficient sanitization is looking for trouble.
> 
> >>   		for (p1 = label.buf; *p1; p1++)
> >> -			if (isspace(*p1))
> >> +			if (!(*p1 & 0x80) && !isalnum(*p1))
> >>   				*(char *)p1 = '-';

While we're thinking of things that could go wrong, note that it's also
possible for the commit message to contain non-UTF-8 characters (if the
user is using a non-UTF-8 encoding), which will cause sadness on Windows
and macOS.  Non-Mac Unix systems aren't a problem here, but then again,
they aren't the reason for this patch.

> > I'm sightly concerned that this opens the possibility for unexpected
> > effects if two different labels get sanitized to the same string. I
> > suspect it's unlikely to happen in practice but doing something like
> > percent encoding non-alphanumeric characters would avoid the problem
> > entirely.
> 
> I see there are "lets make sure it is unique by suffixing "-%d" in
> other codepaths; would that help if this piece of code yields a
> label that is not unique?

I was thinking the same thing.  Since we're being much less lenient on
what's allowed (which is fine), we're at increased risk for collision.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]