threads / patch / 3438

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

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

## tl;dr

8 messages between Feb 22, 2006 and Feb 23, 2006. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Andrew Vasquez· Feb 22, 2006, 18:17 UTC · lore
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:
Show 16 quoted lines
> 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

Junio C Hamano· Feb 22, 2006, 18:26 UTC · re: Andrew Vasquez · lore
Andrew Vasquez <andrew.vasquez@qlogic.com> writes:
Show 9 quoted lines
> 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.
Carl Worth· Feb 22, 2006, 18:44 UTC · re: Andrew Vasquez · lore
On Wed, 22 Feb 2006 10:17:58 -0800, Andrew Vasquez wrote:
Show 12 quoted lines
> 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.
Show 7 quoted lines
> > -	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
Junio C Hamano· Feb 22, 2006, 18:50 UTC · re: Carl Worth · lore
I have this in mind...
---
Show changes to sha1_file.c +2 −1
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++] = '/';
Junio C Hamano· Feb 22, 2006, 19:11 UTC · re: Carl Worth · lore
Carl Worth <cworth@cworth.org> writes:
Show 19 quoted lines
> 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 --
Show changes to sha1_file.c +16 −3
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++] = '/';
Carl Worth· Feb 22, 2006, 19:16 UTC · re: Junio C Hamano · lore
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
Jonas Fonseca· Feb 23, 2006, 00:34 UTC · re: Junio C Hamano · lore
Junio C Hamano <junkio@cox.net> wrote Wed, Feb 22, 2006:
Show 8 quoted lines
> @@ -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
Junio C Hamano· Feb 23, 2006, 01:48 UTC · re: Jonas Fonseca · lore

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

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
Show changes to 6 files +15 −13

commit.c, http-fetch.c, read-tree.c, receive-pack.c, refs.c, sha1_file.c

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

← back to recent threads