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

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

[Kyle J. McKay]

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.

Unfortunately this causes a directory to be passed to git_open_noatime
instead of a file which it happily opens and returns whereupon this
open directory file handle is then passed to mmap.

mmap does not like this and fails with an invalid argument error
at which point the pack-objects process dies prematurely.

To avoid this we preserve the last character of the base member,
set it to '/', make the git_open_noatime call and then restore
it to its previous value which allows pack-objects to function properly.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---

*****

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.

All this stuffing of a single char back and forth into a [-1] index
seems awfully kludgy to me.

However, without this fix and the previous sha1_file fix I would need
to jettison the latest stuff and revert back to 2.1.4.

I suggest that this (or another fix for the problem) go into maint
together with the "sha1_file: fix iterating loose alternate objects"
fix.

*****

 sha1_file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index f575b3cf..235f6ad8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1491,8 +1491,11 @@ static int open_sha1_file(const unsigned char *sha1)
 
 	prepare_alt_odb();
 	for (alt = alt_odb_list; alt; alt = alt->next) {
+		char c = alt->name[-1];
+		alt->name[-1] = '/';
 		fill_sha1_path(alt->name, sha1);
 		fd = git_open_noatime(alt->base);
+		alt->name[-1] = c;
 		if (fd >= 0)
 			return fd;
 		if (most_interesting_errno == ENOENT)
--

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

[Jeff King]

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

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

[Jeff King]

On Sun, Feb 08, 2015 at 07:54:44PM -0500, Jeff King wrote:

> 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.

Here it is. The first patch is a refactoring to allow this,
and the second is the moral equivalent of Jonathon's patch.
These replace a1b47246 on the tip of jk/prune-mtime.

  [1/2]: for_each_loose_file_in_objdir: take an optional strbuf path
  [2/2]: sha1_file: fix iterating loose alternate objects

-Peff

[PATCH 1/2] for_each_loose_file_in_objdir: take an optional strbuf path

[Jeff King]

We feed a root "objdir" path to this iterator function,
which then copies the result into a strbuf, so that it can
repeatedly append the object sub-directories to it. Let's
make it easy for callers to just pass us a strbuf in the
first place.

We leave the original interface as a convenience for callers
who want to just pass a const string like the result of
get_object_directory().

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h     |  9 +++++++++
 sha1_file.c | 31 +++++++++++++++++++++----------
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 51ee856..4743f7e 100644
--- a/cache.h
+++ b/cache.h
@@ -1256,6 +1256,10 @@ extern int unpack_object_header(struct packed_git *, struct pack_window **, off_
  *
  * Any callback that is NULL will be ignored. Callbacks returning non-zero
  * will end the iteration.
+ *
+ * In the "buf" variant, "path" is a strbuf which will also be used as a
+ * scratch buffer, but restored to its original contents before
+ * the function returns.
  */
 typedef int each_loose_object_fn(const unsigned char *sha1,
 				 const char *path,
@@ -1271,6 +1275,11 @@ int for_each_loose_file_in_objdir(const char *path,
 				  each_loose_cruft_fn cruft_cb,
 				  each_loose_subdir_fn subdir_cb,
 				  void *data);
+int for_each_loose_file_in_objdir_buf(struct strbuf *path,
+				      each_loose_object_fn obj_cb,
+				      each_loose_cruft_fn cruft_cb,
+				      each_loose_subdir_fn subdir_cb,
+				      void *data);
 
 /*
  * Iterate over loose and packed objects in both the local
diff --git a/sha1_file.c b/sha1_file.c
index c632641..725de7f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3358,31 +3358,42 @@ static int for_each_file_in_obj_subdir(int subdir_nr,
 	return r;
 }
 
-int for_each_loose_file_in_objdir(const char *path,
+int for_each_loose_file_in_objdir_buf(struct strbuf *path,
 			    each_loose_object_fn obj_cb,
 			    each_loose_cruft_fn cruft_cb,
 			    each_loose_subdir_fn subdir_cb,
 			    void *data)
 {
-	struct strbuf buf = STRBUF_INIT;
-	size_t baselen;
+	size_t baselen = path->len;
 	int r = 0;
 	int i;
 
-	strbuf_addstr(&buf, path);
-	strbuf_addch(&buf, '/');
-	baselen = buf.len;
-
 	for (i = 0; i < 256; i++) {
-		strbuf_addf(&buf, "%02x", i);
-		r = for_each_file_in_obj_subdir(i, &buf, obj_cb, cruft_cb,
+		strbuf_addf(path, "/%02x", i);
+		r = for_each_file_in_obj_subdir(i, path, obj_cb, cruft_cb,
 						subdir_cb, data);
-		strbuf_setlen(&buf, baselen);
+		strbuf_setlen(path, baselen);
 		if (r)
 			break;
 	}
 
+	return r;
+}
+
+int for_each_loose_file_in_objdir(const char *path,
+				  each_loose_object_fn obj_cb,
+				  each_loose_cruft_fn cruft_cb,
+				  each_loose_subdir_fn subdir_cb,
+				  void *data)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int r;
+
+	strbuf_addstr(&buf, path);
+	r = for_each_loose_file_in_objdir_buf(&buf, obj_cb, cruft_cb,
+					      subdir_cb, data);
 	strbuf_release(&buf);
+
 	return r;
 }
 
-- 
2.3.0.rc1.287.g761fd19

[PATCH 2/2] sha1_file: fix iterating loose alternate objects

[Jeff King]

From: Jonathon Mah <me@jonathonmah.com>

The string in 'base' contains a path suffix to a specific object;
when its value is used, the suffix must either be filled (as in
stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or
cleared (as in prepare_packed_git) to avoid junk at the end.

660c889e (sha1_file: add for_each iterators for loose and packed
objects, 2014-10-15) introduced loose_from_alt_odb(), but this did
neither and treated 'base' as a complete path to the "base" object
directory, instead of a pointer to the "base" of the full path
string.

The trailing path after 'base' is still initialized to NUL, hiding
the bug in some common cases.  Additionally the descendent
for_each_file_in_obj_subdir() function swallows ENOENT, so an error
only shows if the alternate's path was last filled with a valid
object (where statting /path/to/existing/00/0bjectfile/00 fails).

Signed-off-by: Jonathon Mah <me@JonathonMah.com>
Helped-by: Kyle J. McKay <mackyle@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I think the S-O-B should still stand, as the code is now a mix of our
work, and the tests are still Jonathon's. But let me know if you do not
want your name attached to this. ;)

I am also happy to build it as a patch on top of the original if that is
simpler.

 sha1_file.c      | 13 ++++++++++---
 t/t5304-prune.sh |  8 ++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 725de7f..a41cc4f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3406,9 +3406,16 @@ static int loose_from_alt_odb(struct alternate_object_database *alt,
 			      void *vdata)
 {
 	struct loose_alt_odb_data *data = vdata;
-	return for_each_loose_file_in_objdir(alt->base,
-					     data->cb, NULL, NULL,
-					     data->data);
+	struct strbuf buf = STRBUF_INIT;
+	int r;
+
+	/* copy base not including trailing '/' */
+	strbuf_add(&buf, alt->base, alt->name - alt->base - 1);
+	r = for_each_loose_file_in_objdir_buf(&buf,
+					      data->cb, NULL, NULL,
+					      data->data);
+	strbuf_release(&buf);
+	return r;
 }
 
 int for_each_loose_object(each_loose_object_fn cb, void *data)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index e32e46d..0794d33 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -253,4 +253,12 @@ test_expect_success 'prune .git/shallow' '
 	test_path_is_missing .git/shallow
 '
 
+test_expect_success 'prune: handle alternate object database' '
+	test_create_repo A &&
+	git -C A commit --allow-empty -m "initial commit" &&
+	git clone --shared A B &&
+	git -C B commit --allow-empty -m "next commit" &&
+	git -C B prune
+'
+
 test_done
-- 
2.3.0.rc1.287.g761fd19

Re: [PATCH 2/2] sha1_file: fix iterating loose alternate objects

[Kyle J. McKay]

On Feb 8, 2015, at 17:15, Jeff King wrote:
[...]
> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> Helped-by: Kyle J. McKay <mackyle@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I think the S-O-B should still stand, as the code is now a mix of our
> work, and the tests are still Jonathon's. But let me know if you do  
> not
> want your name attached to this. ;)

That's fine.

This fix looks much better. :)

Unfortunately I can no longer reproduce the original bug as the  
repository that caused it is no longer in a state that triggers the  
problem (and my backups of it are either slightly too old or slightly  
too new).

-Kyle

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Thanks; looks very sensible.