Re: [PATCH v1 3/4] ref: add symbolic ref content check for files backend

[shejialuo <shejialuo@gmail.com>]

On Thu, Aug 22, 2024 at 10:53:57AM +0200, Patrick Steinhardt wrote:


> > +		if ((space_num || newline_num) && !isspace(*p)) {
> > +			ret = fsck_report_ref(o, report,
> > +					      FSCK_MSG_BAD_REF_CONTENT,
> > +					      "contains non-null garbage");
> > +			goto out;
> > +		}
> > +
> > +		if (*p == '\n') {
> > +			newline_num++;
> > +		} else if (*p == ' ') {
> > +			space_num++;
> > +		}
> > +		p++;
> > +	}
> 
> Can't we replace this with a single `strchr('\n')` call to check for the
> newline and then verify that the next character is a `\0`? The check for
> spaces would then be handled by `check_refname_format()`.
> 

We cannot. Think about this situation.

  "ref: refs/heads/master  \n   "

We find that the next character of '\n' is not '\0'. Then we leave it to
"check_refname_format". But "check_refname_format" will report an error
here, but this is an allowed symref.

But I think using `strchr` is a nice way. I will try to find an elegant
way here to handle this logic here.

> > +	/*
> > +	 * Missing target should not be treated as any error worthy event and
> > +	 * not even warn. It is a common case that a symbolic ref points to a
> > +	 * ref that does not exist yet. If the target ref does not exist, just
> > +	 * skip the check for the file type.
> > +	 */
> > +	if (lstat(pointee_path->buf, &st) < 0)
> > +		goto out;
> > +
> > +	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) {
> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_BAD_SYMREF_POINTEE,
> > +				      "points to an invalid file type");
> > +		goto out;
> > +	}
> 
> What exactly are we guarding against here? Don't we already verify that
> files in `refs/` have the correct type? Or are we checking that it does
> not point to a directory?
> 

When scanning the "refs" directory, we will check the file in the ref
database, but we ignore the directory. So we are checking to know
whether it does not point to a directory. If the ref points to a bad
file type for example "ref/heads/bad-file"

If it is a block type file. We will first report that "refs/heads/bad-file"
is a bad file and then report ref points to bad file
"refs/heads/bad-file".

Actually, I think this is a little redundant here, but we can be
tolerant about this because we need to guard against directory. We need
to consider this situation.

So we could let this be.

> Patrick