Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/27/2014 07:59 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> When reading a symbolic ref via resolve_gitlink_ref_recursive(), check
>>> that the reference name that is pointed at is formatted correctly,
>>> using the same check as resolve_ref_unsafe() uses for non-gitlink
>>> references.  This prevents bogosity like
>>>
>>>     ref: ../../other/file
>>>
>>> from causing problems.
>>
>> I do agree that a textual symref "ref: ../../x/y" that is stored in
>> ".git/HEAD" or in ".git/refs/L" will step outside ".git/" and it is
>> problematic.  But if ".git/refs/heads/a/b/LINK" has "ref: ../../x"
>> in it, shouldn't we interpret it as referring to the ref at
>> "refs/heads/x"?

I've never seen that usage, nor seen it advocated.  Symrefs are not
propagated via the Git protocol, so even if somebody were doing this
privately, it could hardly be a project-wide practice.  I can't think of
a practical use for this feature.  And it would be mildly annoying to
implement.  So my inclination is to forbid it.

> Actually, the textual symrefs have been invented to replace symbolic
> links used for .git/HEAD on symlink-incapable filesystems, and we do
> even not let the filesystem follow symlinks.  The rule we have there
> are like so:
> 
> 		/* Follow "normalized" - ie "refs/.." symlinks by hand */
> 		if (S_ISLNK(st.st_mode)) {
> 			len = readlink(path, buffer, sizeof(buffer)-1);
> 			if (len < 0) {
> 				if (errno == ENOENT || errno == EINVAL)
> 					/* inconsistent with lstat; retry */
> 					goto stat_ref;
> 				else
> 					return NULL;
> 			}
> 			buffer[len] = 0;
> 			if (starts_with(buffer, "refs/") &&
> 					!check_refname_format(buffer, 0)) {
> 				strcpy(refname_buffer, buffer);
> 				refname = refname_buffer;
> 				if (flag)
> 					*flag |= REF_ISSYMREF;
> 				continue;
> 			}
> 		}
> 
> So we should do exactly the same check, I would think, no?

I think you overlooked that if the (starts_with() &&
!check_refname_format()) check fails, execution falls through, ending up
here:

		/*
		 * Anything else, just open it and try to use it as
		 * a ref
		 */
		fd = open(path, O_RDONLY);
		if (fd < 0) {
			if (errno == ENOENT)
				/* inconsistent with lstat; retry */
				goto stat_ref;
			else
				return NULL;
		}
		len = read_in_full(fd, buffer, sizeof(buffer)-1);
		close(fd);
		[...etc...]

This has been the behavior since time immemorial [1].

In fact, another bug (which I probably introduced) is that in the case
of a symlink that points at a non-existent file, this code goes into an
infinite loop due to the "if (errno == ENOENT) goto stat_ref" in the
code that I quoted.  My mistake was forgetting that lstat() is statting
the link whereas open() follows the link, so the success of the former
does not imply that the latter should not ENOENT.

I suggest we fix both problems by making the code behave the way you
*thought* it behaves: symlinks are never followed via the filesystem,
but if the symlink contents have the form of a legitimate refname that
starts with "refs/", then we follow it the same way as we would follow a
textual-style symref.

> In a typical clone, the ".git/refs/remotes/origin/HEAD" textual
> symref stores "ref: refs/remotes/origin/master" and it is neither
> "ref: master" nor "ref: ./master", so it should be sensible to
> insist on "must start with 'refs/' and its format valid."

Yes, we don't even have a notation for "relative refnames" because we
would have no way to distinguish them from "absolute refnames" except
maybe via some artifice like a "./" prefix.

Michael

[1] Where by "time immemorial" I mean "since before I ever touched refs.c".

-- 
Michael Haggerty
mhagger@alum.mit.edu