Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

From: Jeff King <peff@peff.net>
Date: Dec 7, 2022, 06:42 UTC
Message-ID: <Y5A11dOFgHP/ADcS@coredump.intra.peff.net>
In-Reply-To: <9ddfff3585c293c9801570e395b514505796a43f.1670373420.git.jonathantanmy@google.com>

On Tue, Dec 06, 2022 at 04:40:52PM -0800, Jonathan Tan wrote:

Show 9 quoted lines

> Note that in the RHS of this patch's diff, a check for ENOENT that was
> introduced in 3ba7a06552 (A loose object is not corrupt if it cannot
> be read due to EMFILE, 2010-10-28) is also removed. The purpose of this
> check is to avoid a false report of corruption if the errno contains
> something like EMFILE (or anything that is not ENOENT), in which case
> a more generic report is presented. Because, as of this patch, we no
> longer rely on such a heuristic to determine corruption, but surface
> the error message at the point when we read something that we did not
> expect, this check is no longer necessary.

You're right that this will not say "oops, the object is corrupted" when we get EMFILE, etc, which is what 3ba7a06552 was fixing. But I think after your patch, we would also never actually say "could not open $path: too many open descriptors". I don't think that kicked in reliably with the current code, but it seems like something we are losing in this patch.

I.e., I think you'd want to also complain when map_loose_object() returns anything but ENOENT. The errno value there is reliable-ish, though it might be worth spending the extra work to preserve it across the close() calls, just in case. Or if you split the open/mmap as Ævar suggested, then it becomes:

  fd = open_loose_object(r, oid, &path);
  if (fd < 0) {
	if (errno != ENOENT)
		error_errno("unable to open loose object %s", path);
	return -1;
  }

  buf = map_loose_object_1(fd, path);
  if (!buf) {
	/* not much point in complaining here, as xmmap will die on
	 * error. In theory this could catch fstat() problems, but
	 * probably map_loose_object_1() itself should do so, since
	 * it already is special-casing empty files.
	 */
        close(fd);
	return -1;
  }

Show 21 quoted lines

> diff --git a/object-file.c b/object-file.c
> index 596dd049fd..c7a513d123 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,
>   * searching for a loose object named "oid".
>   */
>  static void *map_loose_object_1(struct repository *r, const char *path,
> -			     const struct object_id *oid, unsigned long *size)
> +				const struct object_id *oid, unsigned long *size,
> +				char **mapped_path)
>  {
>  	void *map;
>  	int fd;
> @@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,
>  		fd = git_open(path);
>  	else
>  		fd = open_loose_object(r, oid, &path);
> +	if (mapped_path)
> +		*mapped_path = xstrdup(path);
> +

This introduces an extra malloc/free in every object lookup, even in the success case where we don't even bother using the value. It's probably not really noticeable, but it kind of feels wrong. Especially when open_loose_object() already returns a pointer to long-ish term storage.

One solution is...

Show 8 quoted lines

> @@ -1497,8 +1504,13 @@ static int loose_object_info(struct repository *r,
>  		break;
>  	}
>  
> +	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
> +		die(_("loose object %s (stored in %s) is corrupt"),
> +		    oid_to_hex(oid), mapped_path);
> +

...we could just say "loose object %s is corrupt", which is generally sufficient (outside of alternates, you can only have one copy anyway).

But if we want to retain it, we could just redo the lookup in the error path, which is what the existing code (that you're deleting) does, via stat_loose_object(). That's even more wasteful than an extra malloc/free, but you only pay the cost for a corrupted object. It's racy, of course, but probably not in a meaningful way in practice.

Alternatively, I like Ævar's suggestion to just split map_loose_object(). Then this code would naturally have the path, via calling open_loose_object() itself.

Show 10 quoted lines

> diff --git a/object-store.h b/object-store.h
> index 1be57abaf1..01134ab5ec 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -447,6 +447,9 @@ struct object_info {
>   */
>  #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
>  
> +/* Die if object corruption (not just an object being missing) was detected. */
> +#define OBJECT_INFO_DIE_IF_CORRUPT 64

I have a suspicion that the world would be a better place if these die() calls simply went away, in favor of returning -1 up the stack. But I'm OK leaving it as-is for the sake of trying not to do too many things at once (I probably wouldn't have even mentioned it, except that if we do want to end up there in the long run, we'd eventually have to rip out this new flag and its associated plumbing).

-Peff