# Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found 8 messages from 2006-02-22 to 2006-02-23. Participants: Andrew Vasquez, Junio C Hamano, Carl Worth, Jonas Fonseca. Thread: https://gitlist.dev/t/3438 ## Andrew Vasquez, 2006-02-22 18:17 Subject: Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found Message-ID: <20060222181758.GH3355@andrew-vasquezs-powerbook-g4-15.local> URL: https://gitlist.dev/e/20060222181758.GH3355%40andrew-vasquezs-powerbook-g4-15.local ``` 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 ``` ## Junio C Hamano, 2006-02-22 18:26 Subject: Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found Message-ID: <7vpslfntb0.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vpslfntb0.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060222181758.GH3355@andrew-vasquezs-powerbook-g4-15.local> ``` Andrew Vasquez 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. ``` ## Carl Worth, 2006-02-22 18:44 Subject: Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found Message-ID: <87bqwzs07h.wl%cworth@cworth.org> URL: https://gitlist.dev/e/87bqwzs07h.wl%25cworth%40cworth.org In-Reply-To: <20060222181758.GH3355@andrew-vasquezs-powerbook-g4-15.local> ``` 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 actually does exist, even if /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, 2006-02-22 18:50 Subject: Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found Message-ID: <7vy803mdnm.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vy803mdnm.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <87bqwzs07h.wl%cworth@cworth.org> ``` 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++] = '/'; ``` ## Junio C Hamano, 2006-02-22 19:11 Subject: Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found Message-ID: <7vr75vmcod.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr75vmcod.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <87bqwzs07h.wl%cworth@cworth.org> ``` Carl Worth 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++] = '/'; ``` ## Carl Worth, 2006-02-22 19:16 Subject: Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found Message-ID: <878xs3rypu.wl%cworth@cworth.org> URL: https://gitlist.dev/e/878xs3rypu.wl%25cworth%40cworth.org In-Reply-To: <7vr75vmcod.fsf@assigned-by-dhcp.cox.net> ``` 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, 2006-02-23 00:34 Subject: Re: [PATCH] Prevent git-upload-pack segfault if object cannot be found Message-ID: <20060223003400.GA27800@diku.dk> URL: https://gitlist.dev/e/20060223003400.GA27800%40diku.dk In-Reply-To: <7vr75vmcod.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano 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 ``` ## Junio C Hamano, 2006-02-23 01:48 Subject: [PATCH] Give no terminating LF to error() function. Message-ID: <7v64n6luan.fsf_-_@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v64n6luan.fsf_-_%40assigned-by-dhcp.cox.net In-Reply-To: <20060223003400.GA27800@diku.dk> ``` Signed-off-by: Junio C Hamano --- Jonas Fonseca writes: > Junio C Hamano 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 ```