Re: [PATCH] sha1_file.c: make sure open_sha1_file does not open a directory

[Jeff King <peff@peff.net>]

On Sun, Feb 08, 2015 at 03:05:32PM -0800, Kyle J. McKay wrote:

> Since "sha1_file: fix iterating loose alternate objects", it's possible
> for the base member of the alt_odb_list structure to be NUL terminated
> rather than ending with a '/' when open_sha1_file is called.

Good catch. Users of "struct alternate_object_database" expect to be
able to fill in the "name" field, and have a full path in the "base"
field. That is part of the contract of the struct, and the recent fix
does not live up to that contract inside the for_each_loose_file...
callbacks.

For that reason, I don't think your fix is complete. It fixes _one_
caller to work around this breakage of the contract, but it does not do
anything about the other callers (of which you can find several if you
grep for `fill_sha1_path`). I don't know if those can be hit from this
code path, but it does not matter. We jump to a callback with the NUL
set, so we must assume any arbitrary code can be run.

So either we must amend the contract, so that users of alt->base must
check the termination themselves (i.e., your patch, but extended to all
users of alt->base), or we have to fix for_each_loose_file not to leave
the alt_odb struct in such a broken state. I think I'd prefer the
latter.

> While this patch can be applied without "sha1_file: fix iterating
> loose alternate objects" you cannot even get to the failure this fixes
> without first having that patch applied.

Right. This is literally a bug introduced by that patch. It's OK to
munge alt->name[-1] temporarily, but you have to make sure you are not
calling functions which will look at it while it is munged. The way
refs_from_alternate_cb does it is OK (NUL-terminate, xstrdup, then fix
it; or just xmemdupz the correct length, which we know from alt->name).

Something like:

diff --git a/sha1_file.c b/sha1_file.c
index 9e0c271..7253213 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3396,11 +3396,11 @@ static int loose_from_alt_odb(struct alternate_object_database *alt,
 {
 	struct loose_alt_odb_data *data = vdata;
 	int r;
-	alt->name[-1] = 0;
-	r = for_each_loose_file_in_objdir(alt->base,
+	char *buf = xmemdupz(alt->base, alt->name - alt->base - 1);
+	r = for_each_loose_file_in_objdir(buf,
 					  data->cb, NULL, NULL,
 					  data->data);
-	alt->name[-1] = '/';
+	free(buf);
 	return r;
 }
 

However, the first thing for_each_loose_file_in_objdir is going to do is
stick the path into a strbuf. So perhaps the most sensible thing is to
just teach it to take a strbuf from the caller. I'll work up a patch.

It looks like a1b47246 isn't even in "next" yet, so I'll build it
directly on what is already in master, dropping Jonathan's patch.

-Peff