Re: [PATCH 04/10] packed-backend: add "packed-refs" header consistency check

[shejialuo <shejialuo@gmail.com>]

On Thu, Jan 16, 2025 at 02:57:37PM +0100, Patrick Steinhardt wrote:
> On Sun, Jan 05, 2025 at 09:49:37PM +0800, shejialuo wrote:
> > Add a new flag "safe_object_check" in "fsck_options", when there is
> > anything wrong with the parsing process, set this flag to 0 to avoid
> > checking objects in the later checks.
> 
> Okay, I understand the motivation: a corrupted refdb may be completely
> bogus, so checking its objects may not be sensible.
> 
> For one of the preceding commits I made the suggestion to split out the
> object checks into a generic part instead, as they aren't specific to
> the backend. With such a scheme we could adapt the logic to first do the
> backend-specific checks for the format, and only in case the backend
> looks sane to us we'd execute those generic checks for that specific
> backend. That'd allow us to get rid of the "safe object check" flag.
> 

Yes, I agree with you here. And I won't touch this topic in the next
version. Let me make this patch concentrate on the "packed-ref" format.

> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index d9eb2f8b71..3b11abe5f8 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -1748,12 +1748,100 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s
> >  	return empty_ref_iterator_begin();
> >  }
> >  
> > +static int packed_fsck_ref_next_line(struct fsck_options *o,
> > +				     int line_number, const char *start,
> > +				     const char *eof, const char **eol)
> > +{
> > +	int ret = 0;
> > +
> > +	*eol = memchr(start, '\n', eof - start);
> > +	if (!*eol) {
> > +		struct strbuf packed_entry = STRBUF_INIT;
> > +		struct fsck_ref_report report = { 0 };
> > +
> > +		strbuf_addf(&packed_entry, "packed-refs line %d", line_number);
> > +		report.path = packed_entry.buf;
> > +		ret = fsck_report_ref(o, &report,
> > +				      FSCK_MSG_PACKED_REF_ENTRY_NOT_TERMINATED,
> > +				      "'%.*s' is not terminated with a newline",
> > +				      (int)(eof - start), start);
> > +
> > +		/*
> > +		 * There is no newline but we still want to parse it to the end of
> > +		 * the buffer.
> > +		 */
> > +		*eol = eof;
> 
> I don't quite understand. We've figured out that there isn't a newline,
> so wouldn't that mean that we _are_ at the end of the buffer already?
> 

In the "packed-refs" file, the last line should end with a newline. If
not, this is a fatal error. The motivation why I do this is that for
each line, we could pass the "line_start" and "eol" to check. But if
there is no newline, the "eol" will be NULL. So, I change it to "eof" to
make sure that we could follow the same logic when "eol" is not NULL.

I guess I should not handle this in this function which may cause
confusion here. I will improve this in the next version.

> > +		strbuf_release(&packed_entry);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int packed_fsck_ref_header(struct fsck_options *o, const char *start, const char *eol)
> > +{
> > +	const char *err_fmt = NULL;
> > +	int fsck_msg_id = -1;
> > +
> > +	if (!starts_with(start, "# pack-refs with:")) {
> > +		err_fmt = "'%.*s' does not start with '# pack-refs with:'";
> > +		fsck_msg_id = FSCK_MSG_BAD_PACKED_REF_HEADER;
> > +	} else if (strncmp(start, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER))) {
> > +		err_fmt = "'%.*s' is not the official packed-refs header";
> 
> I wouldn't say "official", because it could totally be that whatever is
> official changes in the future, e.g. when a new format is introduced.
> Unlikely to happen, but saying "unknown packed-refs header" might be a
> bit more future proof.
> 

I will improve this in the next version.

> > +		fsck_msg_id = FSCK_MSG_UNKNOWN_PACKED_REF_HEADER;
> > +	}
> > +
> > +	if (err_fmt && fsck_msg_id >= 0) {
> > +		struct fsck_ref_report report = { 0 };
> > +		report.path = "packed-refs.header";
> > +
> > +		return fsck_report_ref(o, &report, fsck_msg_id, err_fmt,
> > +				       (int)(eol - start), start);
> > +
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int packed_fsck_ref_content(struct fsck_options *o,
> > +				   const char *start, const char *eof)
> > +{
> > +	int line_number = 1;
> > +	const char *eol;
> > +	int ret = 0;
> > +
> > +	ret |= packed_fsck_ref_next_line(o, line_number, start, eof, &eol);
> > +	if (*start == '#') {
> > +		ret |= packed_fsck_ref_header(o, start, eol);
> > +
> > +		start = eol + 1;
> > +		line_number++;
> 
> The header can only appear at the beginning of the file, can't it? But
> we accept it in every line here. We should likely verify that it's
> actually a header and not a line at some random place.
> 

Yes. But we don't accept it in every line. Because in here, we are
getting the first line "start" and "eol" by using
"packed_fsck_ref_next_line". Only it starts with "#", we will check the
header consistency.

> > +	} else {
> > +		struct fsck_ref_report report = { 0 };
> > +		report.path = "packed-refs";
> > +
> > +		ret |= fsck_report_ref(o, &report,
> > +				       FSCK_MSG_PACKED_REF_MISSING_HEADER,
> > +				       "missing header line");
> > +	}
> > +
> > +	/*
> > +	 * If there is anything wrong during the parsing of the "packed-refs"
> > +	 * file, we should not check the object of the refs.
> > +	 */
> > +	if (ret)
> > +		o->safe_object_check = 0;
> > +
> > +
> > +	return ret;
> > +}
> > +
> >  static int packed_fsck(struct ref_store *ref_store,
> >  		       struct fsck_options *o,
> >  		       struct worktree *wt)
> >  {
> >  	struct packed_ref_store *refs = packed_downcast(ref_store,
> >  							REF_STORE_READ, "fsck");
> > +	struct strbuf packed_ref_content = STRBUF_INIT;
> >  	struct stat st;
> >  	int ret = 0;
> >  
> > @@ -1779,7 +1867,24 @@ static int packed_fsck(struct ref_store *ref_store,
> >  		goto cleanup;
> >  	}
> >  
> > +	if (strbuf_read_file(&packed_ref_content, refs->path, 0) < 0) {
> > +		/*
> > +		 * Although we have checked that the file exists, there is a possibility
> > +		 * that it has been removed between the lstat() and the read attempt by
> > +		 * another process. In that case, we should not report an error.
> > +		 */
> > +		if (errno == ENOENT)
> > +			goto cleanup;
> 
> Unlikely, but good to guard us against that condition regardless. It's
> still not entirely race-free though because the file could meanwhile
> have changed into a symlink, and we wouldn't notice now. We could fix
> that by using open(O_NOFOLLOW), fstat the returne file descriptor and
> then use `strbuf_read()` to slurp in the file.
> 

Would this be too complicated for us to avoid race condition and we will
introduce a lot of code to handle above logic. Because there is a
possibility that when finishing reading the file content to the memory,
the file could be changed into a symlink and we cannot notice. So, I
wanna say we can't avoid race condition totally. It would be good if we
avoid race, but what I am concern about here is that we would make the
logic too complicated. So, could we make it unchanged?