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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html