Re: [PATCH v4 3/4] object-file: emit corruption errors when detected

From: Jeff King <peff@peff.net>
Date: Dec 12, 2022, 20:49 UTC
Message-ID: <Y5eT6jodUdNr6hK6@coredump.intra.peff.net>
In-Reply-To: <xmqqv8mkxgd1.fsf@gitster.g>

On Sat, Dec 10, 2022 at 09:16:42AM +0900, Junio C Hamano wrote:

Show 21 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > +	fd = open_loose_object(r, oid, &path);
> > +	if (fd < 0) {
> > +		if (errno != ENOENT)
> > +			error_errno(_("unable to open loose object %s"), path);
> > +		return -1;
> > +	}
> 
> I know there was a discussion in the previous round, but is this use
> of path truly safe?  Currently it may happen to be as long as there
> is at least one element on the odb list, but when thinking things
> through with future-proofing point of view, I do not think assuming
> that path is always computable is a healthy thing to do in the
> longer term.
> 
> Our "struct object_id" may be extended in the future and allow us to
> express "invalid" object name, in which case the error return we get
> may not even be about "loose object file not openable" but "there
> will never be a loose object file for such an invalid object name",
> in which case there won't be any path returned from the function.

Actually, I think it is much worse than that. The code as written above is already buggy (which is my fault, as I suggested it).

In open_loose_object() we'll continue to iterate and pick out the "most interesting errno". But we'll throw away the path that gave us that errno. So we might well say:

  unable to open loose object /some/alternate/12/34abcd: permission denied

when the actual problem is in /main/objdir/12/34abcd.

It's fixable, but with some pain in handling the allocations. I think it would be sufficient to just say:

  error_errno(_("unable to open loose object %s"), oid_to_hex(oid));

here. And possibly put a comment above open_loose_object() that "path" is only guaranteed to point to something sensible when a non-negative value is returned.

-Peff