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

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

[PATCH] Give no terminating LF to error() function.

[Junio C Hamano]

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

  Jonas Fonseca <fonseca@diku.dk> writes:

  > Junio C Hamano <junkio@cox.net> wrote Wed, Feb 22, 2006:
  >> +			error("unable to open object pack directory: %s: %s\n",
  >> +			      path, strerror(errno));
  >
  > No need for the ending \n.

  True.

 commit.c       |    3 ++-
 http-fetch.c   |    8 ++++----
 read-tree.c    |    4 ++--
 receive-pack.c |    4 ++--
 refs.c         |    2 +-
 sha1_file.c    |    7 ++++---
 6 files changed, 15 insertions(+), 13 deletions(-)

8cd486dcab93856c7eafe33338a3df85b79e4a3e
diff --git a/commit.c b/commit.c
index c550a00..06d5439 100644
--- a/commit.c
+++ b/commit.c
@@ -212,7 +212,8 @@ int parse_commit_buffer(struct commit *i
 	if (memcmp(bufptr, "tree ", 5))
 		return error("bogus commit object %s", sha1_to_hex(item->object.sha1));
 	if (get_sha1_hex(bufptr + 5, parent) < 0)
-		return error("bad tree pointer in commit %s\n", sha1_to_hex(item->object.sha1));
+		return error("bad tree pointer in commit %s",
+			     sha1_to_hex(item->object.sha1));
 	item->tree = lookup_tree(parent);
 	if (item->tree)
 		n_refs++;
diff --git a/http-fetch.c b/http-fetch.c
index ce3df5f..8fd9de0 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -130,7 +130,7 @@ static void start_object_request(struct 
 
 	if (obj_req->local < 0) {
 		obj_req->state = ABORTED;
-		error("Couldn't create temporary file %s for %s: %s\n",
+		error("Couldn't create temporary file %s for %s: %s",
 		      obj_req->tmpfile, obj_req->filename, strerror(errno));
 		return;
 	}
@@ -830,9 +830,9 @@ static int fetch_object(struct alt_base 
 				    obj_req->errorstr, obj_req->curl_result,
 				    obj_req->http_code, hex);
 	} else if (obj_req->zret != Z_STREAM_END) {
-		ret = error("File %s (%s) corrupt\n", hex, obj_req->url);
+		ret = error("File %s (%s) corrupt", hex, obj_req->url);
 	} else if (memcmp(obj_req->sha1, obj_req->real_sha1, 20)) {
-		ret = error("File %s has bad hash\n", hex);
+		ret = error("File %s has bad hash", hex);
 	} else if (obj_req->rename < 0) {
 		ret = error("unable to write sha1 filename %s",
 			    obj_req->filename);
@@ -854,7 +854,7 @@ int fetch(unsigned char *sha1)
 		fetch_alternates(alt->base);
 		altbase = altbase->next;
 	}
-	return error("Unable to find %s under %s\n", sha1_to_hex(sha1),
+	return error("Unable to find %s under %s", sha1_to_hex(sha1),
 		     alt->base);
 }
 
diff --git a/read-tree.c b/read-tree.c
index 52f06e3..8be307e 100644
--- a/read-tree.c
+++ b/read-tree.c
@@ -564,7 +564,7 @@ static int twoway_merge(struct cache_ent
 	struct cache_entry *oldtree = src[1], *newtree = src[2];
 
 	if (merge_size != 2)
-		return error("Cannot do a twoway merge of %d trees\n",
+		return error("Cannot do a twoway merge of %d trees",
 			     merge_size);
 
 	if (current) {
@@ -616,7 +616,7 @@ static int oneway_merge(struct cache_ent
 	struct cache_entry *a = src[1];
 
 	if (merge_size != 1)
-		return error("Cannot do a oneway merge of %d trees\n",
+		return error("Cannot do a oneway merge of %d trees",
 			     merge_size);
 
 	if (!a)
diff --git a/receive-pack.c b/receive-pack.c
index eae31e3..2a3db16 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -92,7 +92,7 @@ static int run_update_hook(const char *r
 	case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
 		return error("waitpid is confused");
 	case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
-		return error("%s died of signal\n", update_hook);
+		return error("%s died of signal", update_hook);
 	case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
 		return error("%s died strangely", update_hook);
 	default:
@@ -158,7 +158,7 @@ static int update(struct command *cmd)
 	if (run_update_hook(name, old_hex, new_hex)) {
 		unlink(lock_name);
 		cmd->error_string = "hook declined";
-		return error("hook declined to update %s\n", name);
+		return error("hook declined to update %s", name);
 	}
 	else if (rename(lock_name, name) < 0) {
 		unlink(lock_name);
diff --git a/refs.c b/refs.c
index d01fc39..826ae7a 100644
--- a/refs.c
+++ b/refs.c
@@ -268,7 +268,7 @@ static int write_ref_file(const char *fi
 	char term = '\n';
 	if (write(fd, hex, 40) < 40 ||
 	    write(fd, &term, 1) < 1) {
-		error("Couldn't write %s\n", filename);
+		error("Couldn't write %s", filename);
 		close(fd);
 		return -1;
 	}
diff --git a/sha1_file.c b/sha1_file.c
index 1fd5b79..a80d849 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -564,7 +564,7 @@ static void prepare_packed_git_one(char 
 	dir = opendir(path);
 	if (!dir) {
 		if (errno != ENOENT)
-			error("unable to open object pack directory: %s: %s\n",
+			error("unable to open object pack directory: %s: %s",
 			      path, strerror(errno));
 		return;
 	}
@@ -1513,7 +1513,8 @@ int write_sha1_from_fd(const unsigned ch
 
 	local = mkstemp(tmpfile);
 	if (local < 0)
-		return error("Couldn't open %s for %s\n", tmpfile, sha1_to_hex(sha1));
+		return error("Couldn't open %s for %s",
+			     tmpfile, sha1_to_hex(sha1));
 
 	memset(&stream, 0, sizeof(stream));
 
@@ -1561,7 +1562,7 @@ int write_sha1_from_fd(const unsigned ch
 	}
 	if (memcmp(sha1, real_sha1, 20)) {
 		unlink(tmpfile);
-		return error("File %s has bad hash\n", sha1_to_hex(sha1));
+		return error("File %s has bad hash", sha1_to_hex(sha1));
 	}
 
 	return move_temp_to_file(tmpfile, sha1_file_name(sha1));
-- 
1.2.3.g5978

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