[PATCH] Prevent git-upload-pack segfault if object cannot be found

[PATCH] Prevent git-upload-pack segfault if object cannot be found

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

Signed-off-by: Carl Worth <cworth@cworth.org>

---

It's probably a just-don't-do-that situation, but I did-it by moving a
directory that had already been cached in objects/info/alternates by
clone -l -s. Here's a fix for the segfault that that turned up.

Probably trivial enough to not bother the list with, but I'm still
learning about formatting patches and mails and things, so I'm open to
any feedback if I'm getting anything wrong.

 sha1_file.c   |    4 +++-
 upload-pack.c |    3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 64cf245..1d799f7 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -551,8 +551,10 @@ static void prepare_packed_git_one(char 
 	sprintf(path, "%s/pack", objdir);
 	len = strlen(path);
 	dir = opendir(path);
-	if (!dir)
+	if (!dir) {
+		fprintf(stderr, "unable to open object pack directory: %s: %s\n", path, strerror(errno));
 		return;
+	}
 	path[len++] = '/';
 	while ((de = readdir(dir)) != NULL) {
 		int namelen = strlen(de->d_name);
diff --git a/upload-pack.c b/upload-pack.c
index d198055..3606529 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -216,6 +216,9 @@ static int send_ref(const char *refname,
 	static char *capabilities = "multi_ack";
 	struct object *o = parse_object(sha1);
 
+	if (!o)
+		die("git-upload-pack: cannot find object %s:", sha1_to_hex(sha1));
+
 	if (capabilities)
 		packet_write(1, "%s %s%c%s\n", sha1_to_hex(sha1), refname,
 			0, capabilities);

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Junio C Hamano]

Thanks, this is the last remaining call that did not check its
return value from opendir().

I queued this one and three clone things from you.  They will
appear in "next" first, but also be in 1.2.2.

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Andrew Vasquez]

Commit:

	b5b16990f8b074bd0481ced047b8f8bf66eee6dc
	Prevent git-upload-pack segfault if object cannot be found

is causing some really annoying noise being sent to stderr on some of
my older non-packed repositories:

	$ git status
	# On branch refs/heads/b4
	unable to open object pack directory: .git/objects/pack: No such file or directory
	nothing to commit

	$ git-rev-list --pretty=oneline v8.01.04b3..
	unable to open object pack directory: .git/objects/pack: No such file or directory
	a7401b7109fb2cba7de41a0e30dfe6aa41690ea8 No ZIO.
	...


Here's the relevant hunk:

> diff --git a/sha1_file.c b/sha1_file.c
> index 3d11a9b..f08b1d6 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -551,8 +551,10 @@ static void prepare_packed_git_one(char 
>  	sprintf(path, "%s/pack", objdir);
>  	len = strlen(path);
>  	dir = opendir(path);
> -	if (!dir)
> +	if (!dir) {
> +		fprintf(stderr, "unable to open object pack directory: %s: %s\n", path, strerror(errno));
>  		return;
> +	}
>  	path[len++] = '/';
>  	while ((de = readdir(dir)) != NULL) {
>  		int namelen = strlen(de->d_name);


Could we drop this fprintf to stderr?

Thanks,
Andrew

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Junio C Hamano]

Andrew Vasquez <andrew.vasquez@qlogic.com> writes:

> Commit:
>
> 	b5b16990f8b074bd0481ced047b8f8bf66eee6dc
> 	Prevent git-upload-pack segfault if object cannot be found
>
> is causing some really annoying noise being sent to stderr on some of
> my older non-packed repositories:
>...
> Could we drop this fprintf to stderr?

Thanks for noticing.

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

On Wed, 22 Feb 2006 10:17:58 -0800, Andrew Vasquez wrote:
> Commit:
> 
> 	b5b16990f8b074bd0481ced047b8f8bf66eee6dc
> 	Prevent git-upload-pack segfault if object cannot be found
> 
> is causing some really annoying noise being sent to stderr on some of
> my older non-packed repositories:
> 
> 	$ git status
> 	# On branch refs/heads/b4
> 	unable to open object pack directory: .git/objects/pack: No such file or directory
> 	nothing to commit

Hmm... I can see that would be really annoying.

> > -	if (!dir)
> > +	if (!dir) {
> > +		fprintf(stderr, "unable to open object pack directory: %s: %s\n", path, strerror(errno));
> >  		return;
> > +	}
> 
> Could we drop this fprintf to stderr?

In the case in which I hit the original bug, this message is necessary
to provide information about where the actual problem is. Here is what
that scenario looks like with the message:

$ mkdir original;
$ (cd original; git-init-db; touch foo; git add foo; git commit -m "original")
defaulting to local storage area
Committing initial tree 4d5fcadc293a348e88f777dc0920f11e7d71441c
$ git clone -l -s original clone
$ mv original moved
$ git clone clone again
unable to open object pack directory: /tmp/original/.git/objects/pack: No such file or directory
fatal: git-upload-pack: cannot find object 0153d496df669cbe5cecb665dbe6f95b20461917:
fatal: unexpected EOF
clone-pack from '/tmp/clone/.git' failed.

Here the "cannot find object" message doesn't point to the core
problem, but the "unable to open object pack directory" does contain
the "/tmp/original" path of interest.

One could be a bit more careful about not complaining if <objdir>
actually does exist, even if <objdir>/pack does not.

Or a workaround would be to just run git-init-db in old repositories
to create the pack directory:

	$ rmdir .git/objects/pack/
	$ git status
	unable to open object pack directory: .git/objects/pack: No such file or directory
	nothing to commit
	$ git-init-db
	defaulting to local storage area
	$ git status
	nothing to commit

-Carl



[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Junio C Hamano]

I have this in mind...

---
diff --git a/sha1_file.c b/sha1_file.c
index f08b1d6..dae77fc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -552,7 +552,8 @@ static void prepare_packed_git_one(char 
 	len = strlen(path);
 	dir = opendir(path);
 	if (!dir) {
-		fprintf(stderr, "unable to open object pack directory: %s: %s\n", path, strerror(errno));
+		if (errno != ENOENT)
+			fprintf(stderr, "unable to open object pack directory: %s: %s\n", path, strerror(errno));
 		return;
 	}
 	path[len++] = '/';

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Junio C Hamano]

Carl Worth <cworth@cworth.org> writes:

> In the case in which I hit the original bug, this message is necessary
> to provide information about where the actual problem is. Here is what
> that scenario looks like with the message:
>
> $ mkdir original;
> $ (cd original; git-init-db; touch foo; git add foo; git commit -m "original")
> defaulting to local storage area
> Committing initial tree 4d5fcadc293a348e88f777dc0920f11e7d71441c
> $ git clone -l -s original clone
> $ mv original moved
> $ git clone clone again
> unable to open object pack directory: /tmp/original/.git/objects/pack: No such file or directory
> fatal: git-upload-pack: cannot find object 0153d496df669cbe5cecb665dbe6f95b20461917:
> fatal: unexpected EOF
> clone-pack from '/tmp/clone/.git' failed.
>
> Here the "cannot find object" message doesn't point to the core
> problem, but the "unable to open object pack directory" does contain
> the "/tmp/original" path of interest.

Updated patch.  The root cause of the problem you had was that
alternates was dangling, so we catch that.

-- >8 --
diff --git a/sha1_file.c b/sha1_file.c
index f08b1d6..c08da35 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -247,6 +247,7 @@ static void link_alt_odb_entries(const c
 		for ( ; cp < ep && *cp != sep; cp++)
 			;
 		if (last != cp) {
+			struct stat st;
 			struct alternate_object_database *alt;
 			/* 43 = 40-byte + 2 '/' + terminating NUL */
 			int pfxlen = cp - last;
@@ -269,9 +270,19 @@ static void link_alt_odb_entries(const c
 			}
 			else
 				memcpy(ent->base, last, pfxlen);
+
 			ent->name = ent->base + pfxlen + 1;
-			ent->base[pfxlen] = ent->base[pfxlen + 3] = '/';
-			ent->base[entlen-1] = 0;
+			ent->base[pfxlen + 3] = '/';
+			ent->base[pfxlen] = ent->base[entlen-1] = 0;
+
+			/* Detect cases where alternate disappeared */
+			if (stat(ent->base, &st) || !S_ISDIR(st.st_mode)) {
+				error("object directory %s does not exist; "
+				      "check .git/objects/info/alternates.",
+				      ent->base);
+				goto bad;
+			}
+			ent->base[pfxlen] = '/';
 
 			/* Prevent the common mistake of listing the same
 			 * thing twice, or object directory itself.
@@ -552,7 +563,9 @@ static void prepare_packed_git_one(char 
 	len = strlen(path);
 	dir = opendir(path);
 	if (!dir) {
-		fprintf(stderr, "unable to open object pack directory: %s: %s\n", path, strerror(errno));
+		if (errno != ENOENT)
+			error("unable to open object pack directory: %s: %s\n",
+			      path, strerror(errno));
 		return;
 	}
 	path[len++] = '/';

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]

On Wed, 22 Feb 2006 11:11:14 -0800, Junio C Hamano wrote:
>
> Updated patch.  The root cause of the problem you had was that
> alternates was dangling, so we catch that.

Looks perfect. Thanks for the fix.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found

[Jonas Fonseca]

Junio C Hamano <junkio@cox.net> wrote Wed, Feb 22, 2006:
> @@ -552,7 +563,9 @@ static void prepare_packed_git_one(char 
>  	len = strlen(path);
>  	dir = opendir(path);
>  	if (!dir) {
> -		fprintf(stderr, "unable to open object pack directory: %s: %s\n", path, strerror(errno));
> +		if (errno != ENOENT)
> +			error("unable to open object pack directory: %s: %s\n",
> +			      path, strerror(errno));

No need for the ending \n.

-- 
Jonas Fonseca