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

On 06/27/2014 07:59 PM, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Michael Haggerty [off-list ref] writes:

quoted

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