diff mbox series

[v2,3/4] ref: add symbolic ref content check for files backend

Message ID Zs3558scHssaG_XS@ArchLinux (mailing list archive)
State Superseded
Headers show
Series add ref content check for files backend | expand

Commit Message

shejialuo Aug. 27, 2024, 4:08 p.m. UTC
We have already introduced the checks for regular refs. There is no need
to check the consistency of the target which the symbolic ref points to.
Instead, we just check the content of the symbolic ref itself.

In order to check the content of the symbolic ref, create a function
"files_fsck_symref_target". It will first check whether the "pointee" is
under the "refs/" directory and then we will check the "pointee" itself.

There is no specification about the content of the symbolic ref.
Although we do write "ref: %s\n" to create a symbolic ref by using
"git-symbolic-ref(1)" command. However, this is not mandatory. We still
accept symbolic refs with null trailing garbage. Put it more specific,
the following are correct:

1. "ref: refs/heads/master   "
2. "ref: refs/heads/master   \n  \n"
3. "ref: refs/heads/master\n\n"

But we do not allow any non-null trailing garbage. The following are bad
symbolic contents which will be reported as fsck error by "git-fsck(1)".

1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage  "

In order to provide above checks, we will use "strrchr" to check whether
we have newline in the ref content. Then we will check the name of the
"pointee" is correct by using "check_refname_format". If the function
fails, we need to trim the "pointee" to see whether the null-garbage
causes the function fails. If so, we need to report that there is
null-garbage in the symref content. Otherwise, we should report the user
the "pointee" is bad.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 77 +++++++++++++++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 54 ++++++++++++++++++++++++
 4 files changed, 135 insertions(+)

Comments

Junio C Hamano Aug. 27, 2024, 7:19 p.m. UTC | #1
shejialuo <shejialuo@gmail.com> writes:

> We have already introduced the checks for regular refs. There is no need
> to check the consistency of the target which the symbolic ref points to.
> Instead, we just check the content of the symbolic ref itself.

Just in case you need it in the future, if you ever need to refer to
a symbolic ref in a way that it is clear which of the two kinds you
are talking about, you can say "textual symref" (a regular file
whose contents is "ref: " followed by the target), to contrast them
with "symbolic link used as symref".

In the proposed log message of this commit, all references to
"symbolic ref" talk about textual ones, so I do not see any need to
be extra explicit by saying "textual symref".

> In order to check the content of the symbolic ref, create a function
> "files_fsck_symref_target". It will first check whether the "pointee" is
> under the "refs/" directory and then we will check the "pointee" itself.

Hmph, as the pointee must be within the usual places that you would
find refs (either in refs/ directory or pseudo ref files immediately
below $GIT_DIR), wouldn't we check the pointee when fsck (or "git
refs verify") run and check everything?  The pointee will have its
turn to be checked, and I am not sure why you need to check the
pointee when you find a symbolic ref is pointing at it, which will
lead for it to be checked twice (or more).

I however did not find an additional code to "check the pointee itself"
in the patch, so perhaps it is OK---the only thing that needs fixing
may be the above paragraph if that is the case.

> There is no specification about the content of the symbolic ref.
> Although we do write "ref: %s\n" to create a symbolic ref by using
> "git-symbolic-ref(1)" command. However, this is not mandatory. We still
> accept symbolic refs with null trailing garbage. Put it more specific,
> the following are correct:
>
> 1. "ref: refs/heads/master   "
> 2. "ref: refs/heads/master   \n  \n"
> 3. "ref: refs/heads/master\n\n"
>
> But we do not allow any non-null trailing garbage.

Your use of word "null" is probably too confusing to contributors to
this project.  None of the above has NUL bytes in them.  I think you
want to say something like this:

    A regular file is accepted as a textual symbolic ref if it
    begins with "ref:", followed by zero or more whitespaces,
    followed by the full refname (e.g. "refs/heads/master",
    "refs/tags/v1.0"), followed only by whitespace characters.  We
    always write a single SP after "ref:" and a single LF after the
    full refname, but third-party reimplementations of Git may have
    taken advantage of the looser syntax that is allowed as above.

> The following are bad
> symbolic contents which will be reported as fsck error by "git-fsck(1)".
>
> 1. "ref: refs/heads/master garbage\n"
> 2. "ref: refs/heads/master \n\n\n garbage  "
>
> In order to provide above checks, we will use "strrchr" to check whether
> we have newline in the ref content.

strrchr() to look for only LF is overly strict.  You need to match
what refs/files-backend.c:read_ref_internal() does to the contents
read from such a loose ref file, i.e. strbuf_rtrim().  Any isspace()
bytes are trimmed at the end, including SP, HT, CR and LF.

> +static int files_fsck_symref_target(struct fsck_options *o,
> +				    struct fsck_ref_report *report,
> +				    const char *refname,
> +				    struct strbuf *pointee_name,
> +				    struct strbuf *pointee_path)
> +{
> +	const char *newline_pos = NULL;
> +	const char *p = NULL;
> +	struct stat st;
> +	int ret = 0;
> +
> +	if (!skip_prefix(pointee_name->buf, "refs/", &p)) {
> +
> +		ret = fsck_report_ref(o, report,
> +				      FSCK_MSG_BAD_SYMREF_POINTEE,
> +				      "points to ref outside the refs directory");
> +		goto out;
> +	}
> +
> +	newline_pos = strrchr(p, '\n');
> +	if (!newline_pos || *(newline_pos + 1)) {
> +		ret = fsck_report_ref(o, report,
> +				      FSCK_MSG_REF_MISSING_NEWLINE,
> +				      "missing newline");

If newline_pos is NULL, it is truly a "missing newline" situation.
If I am reading the code correctly, the severity level is set to
INFO, which is good.

If newline_pos is not NULL but newline_pos[1] is not NUL, however,
that is not a "missing newline".  "refs: refs/heads/master\n " would
trigger this report, for example.

As far as I can tell, such a textual symbolic ref is taken as a
valid symbolic ref pointing at "refs/heads/master" by
refs/files-backend.c:read_ref_internal(), so we are trying to detect
a valid but curiously formatted textual symbolic ref file with the
above code?

And strrchr() to find the last LF is not sufficient for that
purpose.  We would never write "refs:  refs/head/master \n",
but the above code will find the LF, be satisified that the LF is
followed by NUL, without realizing that SP there is not something we
would have written!

I am not sure if that is worth detecting that if it is something we
would have written, but if that were the case, then you would
probably need to do

    (1) check the last byte of pointee_name.buf[] to make sure that
        it is LF; and
    (2) remember pointee_name.len, run strbuf_rtrim() on pointee_name,
        and that LF at the end was the only thing that was trimmed by
        checking the pointee_name.len after trimming.

or something like that.  Then you do not have to have an ugly "oh we
need to check again"---the production code would not do that, either.

> +	if (check_refname_format(pointee_name->buf, 0)) {
> +		/*
> +		 * When containing null-garbage, "check_refname_format" will
> +		 * fail, we should trim the "pointee" to check again.
> +		 */
> +		strbuf_rtrim(pointee_name);
> +		if (!check_refname_format(pointee_name->buf, 0)) {
> +			ret = fsck_report_ref(o, report,
> +					      FSCK_MSG_TRAILING_REF_CONTENT,
> +					      "trailing null-garbage");
> +			goto out;
> +		}

IOW, the above "let's retry" feels totally wrong.  You shouldn't
have to do so, and that comes from running check_refname_format()
before rtrimming the pointee_name.

> +		ret = fsck_report_ref(o, report,
> +				      FSCK_MSG_BAD_SYMREF_POINTEE,
> +				      "points to refname with invalid format");
> +	}

Good.  With this check, we know that the referent, if exists, is
well-formed.  The contents of the referent will then be checked just
like all other refs that may not be pointed by any symbolic ref.

> +	/*
> +	 * 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;

Good.

> +	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;

I do not think it is wrong per se, but I am not sure if this check
is needed, either.  When "git fsck" or "git refs verify" is told to
check the loose refs, wouldn't it walk the refs directory and report
such an unusual filesystem entity that is not a regular file,
symbolic link, or a directory as "there is unusual cruft exist
here"?
Patrick Steinhardt Aug. 28, 2024, 12:50 p.m. UTC | #2
On Wed, Aug 28, 2024 at 12:08:07AM +0800, shejialuo wrote:
> We have already introduced the checks for regular refs. There is no need
> to check the consistency of the target which the symbolic ref points to.
> Instead, we just check the content of the symbolic ref itself.
> 
> In order to check the content of the symbolic ref, create a function
> "files_fsck_symref_target". It will first check whether the "pointee" is
> under the "refs/" directory and then we will check the "pointee" itself.
> 
> There is no specification about the content of the symbolic ref.
> Although we do write "ref: %s\n" to create a symbolic ref by using
> "git-symbolic-ref(1)" command. However, this is not mandatory. We still
> accept symbolic refs with null trailing garbage. Put it more specific,
> the following are correct:
> 
> 1. "ref: refs/heads/master   "
> 2. "ref: refs/heads/master   \n  \n"
> 3. "ref: refs/heads/master\n\n"

Now that we're talking about tightening the rules for direct refs, I
wonder whether we'd also want to apply the same rules to symrefs.
Namely, when there is trailing whitespace we should generate an
INFO-level message about that, too. This is mostly for the sake of
consistency.

[snip]
> diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
> index fc074fc571..85fd058c81 100644
> --- a/Documentation/fsck-msgids.txt
> +++ b/Documentation/fsck-msgids.txt
> @@ -28,6 +28,9 @@
>  `badRefName`::
>  	(ERROR) A ref has an invalid format.
>  
> +`badSymrefPointee`::
> +	(ERROR) The pointee of a symref is bad.

I think we'd want to clarify what "bad" is supposed to mean. Like, is a
missing symref pointee bad? If this is about the format of the pointee's
name, we might want to call this "badSymrefPointeeName".

Also, I think we don't typically call the value of a symbolic ref
"pointee", but "target". Searching for "pointee" in our codebase only
gives a single hit, and that one is not related to symbolic refs.

> diff --git a/fsck.h b/fsck.h
> index b85072df57..cbe837f84c 100644
> --- a/fsck.h
> +++ b/fsck.h
> @@ -34,6 +34,7 @@ enum fsck_msg_type {
>  	FUNC(BAD_REF_CONTENT, ERROR) \
>  	FUNC(BAD_REF_FILETYPE, ERROR) \
>  	FUNC(BAD_REF_NAME, ERROR) \
> +	FUNC(BAD_SYMREF_POINTEE, ERROR) \
>  	FUNC(BAD_TIMEZONE, ERROR) \
>  	FUNC(BAD_TREE, ERROR) \
>  	FUNC(BAD_TREE_SHA1, ERROR) \
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 69c00073eb..382c73fcf7 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3434,11 +3434,81 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
>  				  const char *refs_check_dir,
>  				  struct dir_iterator *iter);
>  
> +/*
> + * Check the symref "pointee_name" and "pointee_path". The caller should
> + * make sure that "pointee_path" is absolute. For symbolic ref, "pointee_name"
> + * would be the content after "refs:".
> + */
> +static int files_fsck_symref_target(struct fsck_options *o,
> +				    struct fsck_ref_report *report,
> +				    const char *refname,
> +				    struct strbuf *pointee_name,
> +				    struct strbuf *pointee_path)
> +{
> +	const char *newline_pos = NULL;
> +	const char *p = NULL;
> +	struct stat st;
> +	int ret = 0;
> +
> +	if (!skip_prefix(pointee_name->buf, "refs/", &p)) {
> +
> +		ret = fsck_report_ref(o, report,
> +				      FSCK_MSG_BAD_SYMREF_POINTEE,
> +				      "points to ref outside the refs directory");
> +		goto out;
> +	}
> +
> +	newline_pos = strrchr(p, '\n');
> +	if (!newline_pos || *(newline_pos + 1)) {
> +		ret = fsck_report_ref(o, report,
> +				      FSCK_MSG_REF_MISSING_NEWLINE,
> +				      "missing newline");
> +	}

The second condition `*(newline_pos + 1)` checks whether there is any
data after the newline, doesn't it? That indicates a different kind of
error than "missing newline", namely that there is trailing garbage. I
guess we'd want to report a separate info-level message for this.

Also, shouldn't we use `strchr` instead of `strrchr()`? Otherwise, we're
only checking for trailing garbage after the _last_ newline, not after
the first one.

> +	if (check_refname_format(pointee_name->buf, 0)) {
> +		/*
> +		 * When containing null-garbage, "check_refname_format" will
> +		 * fail, we should trim the "pointee" to check again.
> +		 */
> +		strbuf_rtrim(pointee_name);
> +		if (!check_refname_format(pointee_name->buf, 0)) {
> +			ret = fsck_report_ref(o, report,
> +					      FSCK_MSG_TRAILING_REF_CONTENT,
> +					      "trailing null-garbage");
> +			goto out;
> +		}

Ah, I didn't get at first that we're doing the check a second time here.
As mentioned above, I think we should check for trailing garbage further
up already and more explicitly.

> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> index 7c1910d784..69280795ca 100755
> --- a/t/t0602-reffiles-fsck.sh
> +++ b/t/t0602-reffiles-fsck.sh
> @@ -176,4 +176,58 @@ test_expect_success 'regular ref content should be checked' '
>  	test_cmp expect err
>  '
>  
> +test_expect_success 'symbolic ref content should be checked' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	branch_dir_prefix=.git/refs/heads &&
> +	tag_dir_prefix=.git/refs/tags &&
> +	cd repo &&
> +	git commit --allow-empty -m initial &&
> +	git checkout -b branch-1 &&
> +	git tag tag-1 &&
> +	git checkout -b a/b/branch-2 &&
> +
> +	printf "ref: refs/heads/branch" > $branch_dir_prefix/branch-1-no-newline &&
> +	git refs verify 2>err &&
> +	cat >expect <<-EOF &&
> +	warning: refs/heads/branch-1-no-newline: refMissingNewline: missing newline
> +	EOF
> +	rm $branch_dir_prefix/branch-1-no-newline &&
> +	test_cmp expect err &&

Same comments here as in the preceding patch for the tests.

Patrick
shejialuo Aug. 28, 2024, 3:26 p.m. UTC | #3
On Tue, Aug 27, 2024 at 12:19:11PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > In order to check the content of the symbolic ref, create a function
> > "files_fsck_symref_target". It will first check whether the "pointee" is
> > under the "refs/" directory and then we will check the "pointee" itself.
> 
> Hmph, as the pointee must be within the usual places that you would
> find refs (either in refs/ directory or pseudo ref files immediately
> below $GIT_DIR), wouldn't we check the pointee when fsck (or "git
> refs verify") run and check everything?  The pointee will have its
> turn to be checked, and I am not sure why you need to check the
> pointee when you find a symbolic ref is pointing at it, which will
> lead for it to be checked twice (or more).
> 
> I however did not find an additional code to "check the pointee itself"
> in the patch, so perhaps it is OK---the only thing that needs fixing
> may be the above paragraph if that is the case.
> 

Yes, "we will check the 'pointee'" itself makes the reader confused. I
will fix the above paragraph. Actually we do not check the "pointee",
but check the symref content. Will fix this in the next version.

> > There is no specification about the content of the symbolic ref.
> > Although we do write "ref: %s\n" to create a symbolic ref by using
> > "git-symbolic-ref(1)" command. However, this is not mandatory. We still
> > accept symbolic refs with null trailing garbage. Put it more specific,
> > the following are correct:
> >
> > 1. "ref: refs/heads/master   "
> > 2. "ref: refs/heads/master   \n  \n"
> > 3. "ref: refs/heads/master\n\n"
> >
> > But we do not allow any non-null trailing garbage.
> 
> Your use of word "null" is probably too confusing to contributors to
> this project.  None of the above has NUL bytes in them.  I think you
> want to say something like this:
> 
>     A regular file is accepted as a textual symbolic ref if it
>     begins with "ref:", followed by zero or more whitespaces,
>     followed by the full refname (e.g. "refs/heads/master",
>     "refs/tags/v1.0"), followed only by whitespace characters.  We
>     always write a single SP after "ref:" and a single LF after the
>     full refname, but third-party reimplementations of Git may have
>     taken advantage of the looser syntax that is allowed as above.
> 

Thanks for your suggestion. I will improve this in the next version.

> > The following are bad
> > symbolic contents which will be reported as fsck error by "git-fsck(1)".
> >
> > 1. "ref: refs/heads/master garbage\n"
> > 2. "ref: refs/heads/master \n\n\n garbage  "
> >
> > In order to provide above checks, we will use "strrchr" to check whether
> > we have newline in the ref content.
> 
> strrchr() to look for only LF is overly strict.  You need to match
> what refs/files-backend.c:read_ref_internal() does to the contents
> read from such a loose ref file, i.e. strbuf_rtrim().  Any isspace()
> bytes are trimmed at the end, including SP, HT, CR and LF.
> 

I will look into how "strbuf_rtrim" does to see whether we can reuse
some functions to avoid repetition.

> > +static int files_fsck_symref_target(struct fsck_options *o,
> > +				    struct fsck_ref_report *report,
> > +				    const char *refname,
> > +				    struct strbuf *pointee_name,
> > +				    struct strbuf *pointee_path)
> > +{
> > +	const char *newline_pos = NULL;
> > +	const char *p = NULL;
> > +	struct stat st;
> > +	int ret = 0;
> > +
> > +	if (!skip_prefix(pointee_name->buf, "refs/", &p)) {
> > +
> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_BAD_SYMREF_POINTEE,
> > +				      "points to ref outside the refs directory");
> > +		goto out;
> > +	}
> > +
> > +	newline_pos = strrchr(p, '\n');
> > +	if (!newline_pos || *(newline_pos + 1)) {
> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_REF_MISSING_NEWLINE,
> > +				      "missing newline");
> 
> If newline_pos is NULL, it is truly a "missing newline" situation.
> If I am reading the code correctly, the severity level is set to
> INFO, which is good.
> 
> If newline_pos is not NULL but newline_pos[1] is not NUL, however,
> that is not a "missing newline".  "refs: refs/heads/master\n " would
> trigger this report, for example.
> 

When I design this, I actually consider "ref: refs/heads/master\n " is
still missing the newline. And then we also report that it has garbage.
I think "ref: refs/heads/master\n \n" is not missing the newline. But, I
don't think this is good.

I will find a good way to handle this.

> As far as I can tell, such a textual symbolic ref is taken as a
> valid symbolic ref pointing at "refs/heads/master" by
> refs/files-backend.c:read_ref_internal(), so we are trying to detect
> a valid but curiously formatted textual symbolic ref file with the
> above code?

Yes, these situations will be taken as a valid symbolic ref but actually
there are something wrong. So this is what we need to care about.

> 
> And strrchr() to find the last LF is not sufficient for that
> purpose.  We would never write "refs:  refs/head/master \n",
> but the above code will find the LF, be satisified that the LF is
> followed by NUL, without realizing that SP there is not something we
> would have written!

I totally ignored this situation, and in current patch, we cannot check
this. I know why Patrick lets me use "strchr" but not "strrchr". I think
we should find the last '\n'. But instead we need to find the first
'\n'. However, in this example, we will still fail by using "strchr".
This part should be totally re-designed.

> 
> I am not sure if that is worth detecting that if it is something we
> would have written, but if that were the case, then you would
> probably need to do
> 
>     (1) check the last byte of pointee_name.buf[] to make sure that
>         it is LF; and
>     (2) remember pointee_name.len, run strbuf_rtrim() on pointee_name,
>         and that LF at the end was the only thing that was trimmed by
>         checking the pointee_name.len after trimming.
> 
> or something like that.  Then you do not have to have an ugly "oh we
> need to check again"---the production code would not do that, either.
> 

Yes, this is a good idea.

> > +	if (check_refname_format(pointee_name->buf, 0)) {
> > +		/*
> > +		 * When containing null-garbage, "check_refname_format" will
> > +		 * fail, we should trim the "pointee" to check again.
> > +		 */
> > +		strbuf_rtrim(pointee_name);
> > +		if (!check_refname_format(pointee_name->buf, 0)) {
> > +			ret = fsck_report_ref(o, report,
> > +					      FSCK_MSG_TRAILING_REF_CONTENT,
> > +					      "trailing null-garbage");
> > +			goto out;
> > +		}
> 
> IOW, the above "let's retry" feels totally wrong.  You shouldn't
> have to do so, and that comes from running check_refname_format()
> before rtrimming the pointee_name.
> 

Yes, actually, I have thought I could compare the length change after
executing the "strbuf_rtrim". I don't want to create two new variables,
so I call "check_refname_format" twice.

Will fix this in the next version.

> > +	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;
> 
> I do not think it is wrong per se, but I am not sure if this check
> is needed, either.  When "git fsck" or "git refs verify" is told to
> check the loose refs, wouldn't it walk the refs directory and report
> such an unusual filesystem entity that is not a regular file,
> symbolic link, or a directory as "there is unusual cruft exist
> here"?

When setting up the infrastructure, actually we DO report filesystem
entity that is not a regular file or symbolic link like the following:

    if (S_ISDIR(iter->st.st_mode)) {
        continue;
    } else if (S_ISREG(iter->st.st_mode) ||
               S_ISLNK(iter->st.st_mode)) {
        ...;
    } else {
      // report file system error
    }

We do not check the directory, because the directory will be always
valid in the filesystem. we could not say that

  "refs/heads/a/" is a bad ref.

So, this check mainly need to check whether the symref points to a
directory. Actually, Patrick has also gave the review about this
question in the previous version:

> 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?

However, we should remove this line, because "check_refname_format" will
take care for us.

  git check-ref-format 'refs/heads/'

It will generate an error. So, we could entirely remove this line and
let "check_refname_format" do this. And we could also remove the

    if (lstat(pointee_path->buf, &st) < 0)
        goto out;

The code will be much more clean.

Thanks,
Jialuo
shejialuo Aug. 28, 2024, 3:36 p.m. UTC | #4
On Wed, Aug 28, 2024 at 02:50:09PM +0200, Patrick Steinhardt wrote:
> On Wed, Aug 28, 2024 at 12:08:07AM +0800, shejialuo wrote:
> > We have already introduced the checks for regular refs. There is no need
> > to check the consistency of the target which the symbolic ref points to.
> > Instead, we just check the content of the symbolic ref itself.
> > 
> > In order to check the content of the symbolic ref, create a function
> > "files_fsck_symref_target". It will first check whether the "pointee" is
> > under the "refs/" directory and then we will check the "pointee" itself.
> > 
> > There is no specification about the content of the symbolic ref.
> > Although we do write "ref: %s\n" to create a symbolic ref by using
> > "git-symbolic-ref(1)" command. However, this is not mandatory. We still
> > accept symbolic refs with null trailing garbage. Put it more specific,
> > the following are correct:
> > 
> > 1. "ref: refs/heads/master   "
> > 2. "ref: refs/heads/master   \n  \n"
> > 3. "ref: refs/heads/master\n\n"
> 
> Now that we're talking about tightening the rules for direct refs, I
> wonder whether we'd also want to apply the same rules to symrefs.
> Namely, when there is trailing whitespace we should generate an
> INFO-level message about that, too. This is mostly for the sake of
> consistency.
> 

Yes, actually this patch does this. I think I need to mention we reuse
the "FSCK_INFO" message id defined in the [PATCH v2 2/4].

> [snip]
> > diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
> > index fc074fc571..85fd058c81 100644
> > --- a/Documentation/fsck-msgids.txt
> > +++ b/Documentation/fsck-msgids.txt
> > @@ -28,6 +28,9 @@
> >  `badRefName`::
> >  	(ERROR) A ref has an invalid format.
> >  
> > +`badSymrefPointee`::
> > +	(ERROR) The pointee of a symref is bad.
> 
> I think we'd want to clarify what "bad" is supposed to mean. Like, is a
> missing symref pointee bad? If this is about the format of the pointee's
> name, we might want to call this "badSymrefPointeeName".
> 

I agree, bad is too general here, we need to make it concrete.

> Also, I think we don't typically call the value of a symbolic ref
> "pointee", but "target". Searching for "pointee" in our codebase only
> gives a single hit, and that one is not related to symbolic refs.
> 

Thanks, I will fix this in the next version.

> > +/*
> > + * Check the symref "pointee_name" and "pointee_path". The caller should
> > + * make sure that "pointee_path" is absolute. For symbolic ref, "pointee_name"
> > + * would be the content after "refs:".
> > + */
> > +static int files_fsck_symref_target(struct fsck_options *o,
> > +				    struct fsck_ref_report *report,
> > +				    const char *refname,
> > +				    struct strbuf *pointee_name,
> > +				    struct strbuf *pointee_path)
> > +{
> > +	const char *newline_pos = NULL;
> > +	const char *p = NULL;
> > +	struct stat st;
> > +	int ret = 0;
> > +
> > +	if (!skip_prefix(pointee_name->buf, "refs/", &p)) {
> > +
> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_BAD_SYMREF_POINTEE,
> > +				      "points to ref outside the refs directory");
> > +		goto out;
> > +	}
> > +
> > +	newline_pos = strrchr(p, '\n');
> > +	if (!newline_pos || *(newline_pos + 1)) {
> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_REF_MISSING_NEWLINE,
> > +				      "missing newline");
> > +	}
> 
> The second condition `*(newline_pos + 1)` checks whether there is any
> data after the newline, doesn't it? That indicates a different kind of
> error than "missing newline", namely that there is trailing garbage. I
> guess we'd want to report a separate info-level message for this.
> 
> Also, shouldn't we use `strchr` instead of `strrchr()`? Otherwise, we're
> only checking for trailing garbage after the _last_ newline, not after
> the first one.
> 

Yes, I totally made a mistake here. I will try to think about a new
design. I have already replied to Junio like the following:

> > And strrchr() to find the last LF is not sufficient for that
> > purpose.  We would never write "refs:  refs/head/master \n",
> > but the above code will find the LF, be satisified that the LF is
> > followed by NUL, without realizing that SP there is not something we
> > would have written!

> I totally ignored this situation, and in current patch, we cannot check
> this. I know why Patrick lets me use "strchr" but not "strrchr". I think
> we should find the last '\n'. But instead we need to find the first
> '\n'. However, in this example, we will still fail by using "strchr".
> This part should be totally re-designed.

> > +	if (check_refname_format(pointee_name->buf, 0)) {
> > +		/*
> > +		 * When containing null-garbage, "check_refname_format" will
> > +		 * fail, we should trim the "pointee" to check again.
> > +		 */
> > +		strbuf_rtrim(pointee_name);
> > +		if (!check_refname_format(pointee_name->buf, 0)) {
> > +			ret = fsck_report_ref(o, report,
> > +					      FSCK_MSG_TRAILING_REF_CONTENT,
> > +					      "trailing null-garbage");
> > +			goto out;
> > +		}
> 
> Ah, I didn't get at first that we're doing the check a second time here.
> As mentioned above, I think we should check for trailing garbage further
> up already and more explicitly.
> 

Well, I guess the implementation about this is totally wrong, which will
make the reviewers hard to understand. I will drop this way to
explicitly check the garbage.

Thanks,
Jialuo
Junio C Hamano Aug. 28, 2024, 3:41 p.m. UTC | #5
Patrick Steinhardt <ps@pks.im> writes:

> Also, I think we don't typically call the value of a symbolic ref
> "pointee", but "target". Searching for "pointee" in our codebase only
> gives a single hit, and that one is not related to symbolic refs.

Yesterday while I was studying for reviewing this series, I saw some
existing code that call them "referent".  There may also be "target".

>> +	if (!newline_pos || *(newline_pos + 1)) {
>> +		ret = fsck_report_ref(o, report,
>> +				      FSCK_MSG_REF_MISSING_NEWLINE,
>> +				      "missing newline");
>> +	}
>
> The second condition `*(newline_pos + 1)` checks whether there is any
> data after the newline, doesn't it? That indicates a different kind of
> error than "missing newline", namely that there is trailing garbage. I
> guess we'd want to report a separate info-level message for this.
>
> Also, shouldn't we use `strchr` instead of `strrchr()`? Otherwise, we're
> only checking for trailing garbage after the _last_ newline, not after
> the first one.

None of the above.  It should strbuf_rtrim() and if we removed
anything but just a single terminating LF, we are looking at
something we wouldn't ahve written.  The next check_refname_format()
call would then find "trailing garbage".

 - "refs/heads/master \n " gets rtrimmed to "refs/heads/master",
   which is "valid but curious".

 - "refs/heads/main trash\n " becomes "refs/heads/main trash",
   which is outright bad.
Patrick Steinhardt Aug. 29, 2024, 10:11 a.m. UTC | #6
On Wed, Aug 28, 2024 at 08:41:08AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Also, I think we don't typically call the value of a symbolic ref
> > "pointee", but "target". Searching for "pointee" in our codebase only
> > gives a single hit, and that one is not related to symbolic refs.
> 
> Yesterday while I was studying for reviewing this series, I saw some
> existing code that call them "referent".  There may also be "target".

Ah, true, I totally forgot about "referent". I guess we use both, but it
would of course be great if we only had a single term to refer them.
Referent seems to be used more widely, at least in the refs subsystem.

> >> +	if (!newline_pos || *(newline_pos + 1)) {
> >> +		ret = fsck_report_ref(o, report,
> >> +				      FSCK_MSG_REF_MISSING_NEWLINE,
> >> +				      "missing newline");
> >> +	}
> >
> > The second condition `*(newline_pos + 1)` checks whether there is any
> > data after the newline, doesn't it? That indicates a different kind of
> > error than "missing newline", namely that there is trailing garbage. I
> > guess we'd want to report a separate info-level message for this.
> >
> > Also, shouldn't we use `strchr` instead of `strrchr()`? Otherwise, we're
> > only checking for trailing garbage after the _last_ newline, not after
> > the first one.
> 
> None of the above.  It should strbuf_rtrim() and if we removed
> anything but just a single terminating LF, we are looking at
> something we wouldn't ahve written.  The next check_refname_format()
> call would then find "trailing garbage".

Fair.

>  - "refs/heads/master \n " gets rtrimmed to "refs/heads/master",
>    which is "valid but curious".

Okay. This _may_ be something to generate an info message for, mostly in
the same spirit as we want to do it for direct refs.

>  - "refs/heads/main trash\n " becomes "refs/heads/main trash",
>    which is outright bad.

Yeah, this one should be an error indeed.

Patrick
diff mbox series

Patch

diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
index fc074fc571..85fd058c81 100644
--- a/Documentation/fsck-msgids.txt
+++ b/Documentation/fsck-msgids.txt
@@ -28,6 +28,9 @@ 
 `badRefName`::
 	(ERROR) A ref has an invalid format.
 
+`badSymrefPointee`::
+	(ERROR) The pointee of a symref is bad.
+
 `badTagName`::
 	(INFO) A tag has an invalid format.
 
diff --git a/fsck.h b/fsck.h
index b85072df57..cbe837f84c 100644
--- a/fsck.h
+++ b/fsck.h
@@ -34,6 +34,7 @@  enum fsck_msg_type {
 	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
+	FUNC(BAD_SYMREF_POINTEE, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \
 	FUNC(BAD_TREE, ERROR) \
 	FUNC(BAD_TREE_SHA1, ERROR) \
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 69c00073eb..382c73fcf7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3434,11 +3434,81 @@  typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
 				  const char *refs_check_dir,
 				  struct dir_iterator *iter);
 
+/*
+ * Check the symref "pointee_name" and "pointee_path". The caller should
+ * make sure that "pointee_path" is absolute. For symbolic ref, "pointee_name"
+ * would be the content after "refs:".
+ */
+static int files_fsck_symref_target(struct fsck_options *o,
+				    struct fsck_ref_report *report,
+				    const char *refname,
+				    struct strbuf *pointee_name,
+				    struct strbuf *pointee_path)
+{
+	const char *newline_pos = NULL;
+	const char *p = NULL;
+	struct stat st;
+	int ret = 0;
+
+	if (!skip_prefix(pointee_name->buf, "refs/", &p)) {
+
+		ret = fsck_report_ref(o, report,
+				      FSCK_MSG_BAD_SYMREF_POINTEE,
+				      "points to ref outside the refs directory");
+		goto out;
+	}
+
+	newline_pos = strrchr(p, '\n');
+	if (!newline_pos || *(newline_pos + 1)) {
+		ret = fsck_report_ref(o, report,
+				      FSCK_MSG_REF_MISSING_NEWLINE,
+				      "missing newline");
+	}
+
+	if (check_refname_format(pointee_name->buf, 0)) {
+		/*
+		 * When containing null-garbage, "check_refname_format" will
+		 * fail, we should trim the "pointee" to check again.
+		 */
+		strbuf_rtrim(pointee_name);
+		if (!check_refname_format(pointee_name->buf, 0)) {
+			ret = fsck_report_ref(o, report,
+					      FSCK_MSG_TRAILING_REF_CONTENT,
+					      "trailing null-garbage");
+			goto out;
+		}
+
+		ret = fsck_report_ref(o, report,
+				      FSCK_MSG_BAD_SYMREF_POINTEE,
+				      "points to refname with invalid format");
+	}
+
+	/*
+	 * 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;
+	}
+
+out:
+	return ret;
+}
+
 static int files_fsck_refs_content(struct ref_store *ref_store,
 				   struct fsck_options *o,
 				   const char *refs_check_dir,
 				   struct dir_iterator *iter)
 {
+	struct strbuf pointee_path = STRBUF_INIT;
 	struct strbuf ref_content = STRBUF_INIT;
 	struct strbuf referent = STRBUF_INIT;
 	struct strbuf refname = STRBUF_INIT;
@@ -3482,6 +3552,12 @@  static int files_fsck_refs_content(struct ref_store *ref_store,
 						      "trailing garbage in ref");
 				goto cleanup;
 			}
+		} else {
+			strbuf_addf(&pointee_path, "%s/%s",
+				    ref_store->gitdir, referent.buf);
+			ret = files_fsck_symref_target(o, &report, refname.buf,
+						       &referent,
+						       &pointee_path);
 		}
 		goto cleanup;
 	}
@@ -3490,6 +3566,7 @@  static int files_fsck_refs_content(struct ref_store *ref_store,
 	strbuf_release(&refname);
 	strbuf_release(&ref_content);
 	strbuf_release(&referent);
+	strbuf_release(&pointee_path);
 	return ret;
 }
 
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index 7c1910d784..69280795ca 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -176,4 +176,58 @@  test_expect_success 'regular ref content should be checked' '
 	test_cmp expect err
 '
 
+test_expect_success 'symbolic ref content should be checked' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	branch_dir_prefix=.git/refs/heads &&
+	tag_dir_prefix=.git/refs/tags &&
+	cd repo &&
+	git commit --allow-empty -m initial &&
+	git checkout -b branch-1 &&
+	git tag tag-1 &&
+	git checkout -b a/b/branch-2 &&
+
+	printf "ref: refs/heads/branch" > $branch_dir_prefix/branch-1-no-newline &&
+	git refs verify 2>err &&
+	cat >expect <<-EOF &&
+	warning: refs/heads/branch-1-no-newline: refMissingNewline: missing newline
+	EOF
+	rm $branch_dir_prefix/branch-1-no-newline &&
+	test_cmp expect err &&
+
+	printf "ref: refs/heads/branch     " > $branch_dir_prefix/a/b/branch-trailing &&
+	git refs verify 2>err &&
+	cat >expect <<-EOF &&
+	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
+	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
+	EOF
+	rm $branch_dir_prefix/a/b/branch-trailing &&
+	test_cmp expect err &&
+
+	printf "ref: refs/heads/branch\n\n" > $branch_dir_prefix/a/b/branch-trailing &&
+	git refs verify 2>err &&
+	cat >expect <<-EOF &&
+	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
+	EOF
+	rm $branch_dir_prefix/a/b/branch-trailing &&
+	test_cmp expect err &&
+
+	printf "ref: refs/heads/branch \n\n " > $branch_dir_prefix/a/b/branch-trailing &&
+	git refs verify 2>err &&
+	cat >expect <<-EOF &&
+	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
+	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
+	EOF
+	rm $branch_dir_prefix/a/b/branch-trailing &&
+	test_cmp expect err &&
+
+	printf "ref: refs/heads/.branch\n" > $branch_dir_prefix/branch-2-bad &&
+	test_must_fail git refs verify 2>err &&
+	cat >expect <<-EOF &&
+	error: refs/heads/branch-2-bad: badSymrefPointee: points to refname with invalid format
+	EOF
+	rm $branch_dir_prefix/branch-2-bad &&
+	test_cmp expect err
+'
+
 test_done