{"thread":{"id":"38532","subject":"[PATCH] sha1_file.c: make sure open_sha1_file does not open a directory","startedAt":"2015-02-08T23:05:32Z","lastAt":"2015-02-09T18:48:40Z","messageCount":7,"participants":["Kyle J. McKay","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"255788","messageId":"b0993cc1fcac290d7506b24942af300@74d39fa044aa309eaea14b9f57fe79c","threadId":"38532","inReplyTo":null,"subject":"[PATCH] sha1_file.c: make sure open_sha1_file does not open a directory","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-08T23:05:32Z","receivedAt":"2015-02-08T23:05:32Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since \"sha1_file: fix iterating loose alternate objects\", it's possible\nfor the base member of the alt_odb_list structure to be NUL terminated\nrather than ending with a '/' when open_sha1_file is called.\n\nUnfortunately this causes a directory to be passed to git_open_noatime\ninstead of a file which it happily opens and returns whereupon this\nopen directory file handle is then passed to mmap.\n\nmmap does not like this and fails with an invalid argument error\nat which point the pack-objects process dies prematurely.\n\nTo avoid this we preserve the last character of the base member,\nset it to '/', make the git_open_noatime call and then restore\nit to its previous value which allows pack-objects to function properly.\n\nSigned-off-by: Kyle J. McKay \n---\n\n*****\n\nWhile this patch can be applied without \"sha1_file: fix iterating\nloose alternate objects\" you cannot even get to the failure this fixes\nwithout first having that patch applied.\n\nAll this stuffing of a single char back and forth into a [-1] index\nseems awfully kludgy to me.\n\nHowever, without this fix and the previous sha1_file fix I would need\nto jettison the latest stuff and revert back to 2.1.4.\n\nI suggest that this (or another fix for the problem) go into maint\ntogether with the \"sha1_file: fix iterating loose alternate objects\"\nfix.\n\n*****\n\n sha1_file.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f575b3cf..235f6ad8 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1491,8 +1491,11 @@ static int open_sha1_file(const unsigned char *sha1)\n \n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n+\t\tchar c = alt->name[-1];\n+\t\talt->name[-1] = '/';\n \t\tfill_sha1_path(alt->name, sha1);\n \t\tfd = git_open_noatime(alt->base);\n+\t\talt->name[-1] = c;\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n--\n"},{"id":"255790","messageId":"20150209005444.GA16827@peff.net","threadId":"38532","inReplyTo":"b0993cc1fcac290d7506b24942af300@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] sha1_file.c: make sure open_sha1_file does not open a directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-09T00:54:44Z","receivedAt":"2015-02-09T00:54:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 08, 2015 at 03:05:32PM -0800, Kyle J. McKay wrote:\n\n> Since \"sha1_file: fix iterating loose alternate objects\", it's possible\n> for the base member of the alt_odb_list structure to be NUL terminated\n> rather than ending with a '/' when open_sha1_file is called.\n\nGood catch. Users of \"struct alternate_object_database\" expect to be\nable to fill in the \"name\" field, and have a full path in the \"base\"\nfield. That is part of the contract of the struct, and the recent fix\ndoes not live up to that contract inside the for_each_loose_file...\ncallbacks.\n\nFor that reason, I don't think your fix is complete. It fixes _one_\ncaller to work around this breakage of the contract, but it does not do\nanything about the other callers (of which you can find several if you\ngrep for `fill_sha1_path`). I don't know if those can be hit from this\ncode path, but it does not matter. We jump to a callback with the NUL\nset, so we must assume any arbitrary code can be run.\n\nSo either we must amend the contract, so that users of alt->base must\ncheck the termination themselves (i.e., your patch, but extended to all\nusers of alt->base), or we have to fix for_each_loose_file not to leave\nthe alt_odb struct in such a broken state. I think I'd prefer the\nlatter.\n\n> While this patch can be applied without \"sha1_file: fix iterating\n> loose alternate objects\" you cannot even get to the failure this fixes\n> without first having that patch applied.\n\nRight. This is literally a bug introduced by that patch. It's OK to\nmunge alt->name[-1] temporarily, but you have to make sure you are not\ncalling functions which will look at it while it is munged. The way\nrefs_from_alternate_cb does it is OK (NUL-terminate, xstrdup, then fix\nit; or just xmemdupz the correct length, which we know from alt->name).\n\nSomething like:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9e0c271..7253213 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3396,11 +3396,11 @@ static int loose_from_alt_odb(struct alternate_object_database *alt,\n {\n \tstruct loose_alt_odb_data *data = vdata;\n \tint r;\n-\talt->name[-1] = 0;\n-\tr = for_each_loose_file_in_objdir(alt->base,\n+\tchar *buf = xmemdupz(alt->base, alt->name - alt->base - 1);\n+\tr = for_each_loose_file_in_objdir(buf,\n \t\t\t\t\t data->cb, NULL, NULL,\n \t\t\t\t\t data->data);\n-\talt->name[-1] = '/';\n+\tfree(buf);\n \treturn r;\n }\n \n\nHowever, the first thing for_each_loose_file_in_objdir is going to do is\nstick the path into a strbuf. So perhaps the most sensible thing is to\njust teach it to take a strbuf from the caller. I'll work up a patch.\n\nIt looks like a1b47246 isn't even in \"next\" yet, so I'll build it\ndirectly on what is already in master, dropping Jonathan's patch.\n\n-Peff\n"},{"id":"255791","messageId":"20150209011159.GA21072@peff.net","threadId":"38532","inReplyTo":"20150209005444.GA16827@peff.net","subject":"Re: [PATCH] sha1_file.c: make sure open_sha1_file does not open a directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-09T01:12:00Z","receivedAt":"2015-02-09T01:12:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 08, 2015 at 07:54:44PM -0500, Jeff King wrote:\n\n> However, the first thing for_each_loose_file_in_objdir is going to do is\n> stick the path into a strbuf. So perhaps the most sensible thing is to\n> just teach it to take a strbuf from the caller. I'll work up a patch.\n> \n> It looks like a1b47246 isn't even in \"next\" yet, so I'll build it\n> directly on what is already in master, dropping Jonathan's patch.\n\nHere it is. The first patch is a refactoring to allow this,\nand the second is the moral equivalent of Jonathon's patch.\nThese replace a1b47246 on the tip of jk/prune-mtime.\n\n [1/2]: for_each_loose_file_in_objdir: take an optional strbuf path\n [2/2]: sha1_file: fix iterating loose alternate objects\n\n-Peff\n"},{"id":"255792","messageId":"20150209011321.GA21123@peff.net","threadId":"38532","inReplyTo":"20150209011159.GA21072@peff.net","subject":"[PATCH 1/2] for_each_loose_file_in_objdir: take an optional strbuf path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-09T01:13:22Z","receivedAt":"2015-02-09T01:13:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We feed a root \"objdir\" path to this iterator function,\nwhich then copies the result into a strbuf, so that it can\nrepeatedly append the object sub-directories to it. Let's\nmake it easy for callers to just pass us a strbuf in the\nfirst place.\n\nWe leave the original interface as a convenience for callers\nwho want to just pass a const string like the result of\nget_object_directory().\n\nSigned-off-by: Jeff King \n---\n cache.h | 9 +++++++++\n sha1_file.c | 31 +++++++++++++++++++++----------\n 2 files changed, 30 insertions(+), 10 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 51ee856..4743f7e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1256,6 +1256,10 @@ extern int unpack_object_header(struct packed_git *, struct pack_window **, off_\n *\n * Any callback that is NULL will be ignored. Callbacks returning non-zero\n * will end the iteration.\n+ *\n+ * In the \"buf\" variant, \"path\" is a strbuf which will also be used as a\n+ * scratch buffer, but restored to its original contents before\n+ * the function returns.\n */\n typedef int each_loose_object_fn(const unsigned char *sha1,\n \t\t\t\t const char *path,\n@@ -1271,6 +1275,11 @@ int for_each_loose_file_in_objdir(const char *path,\n \t\t\t\t each_loose_cruft_fn cruft_cb,\n \t\t\t\t each_loose_subdir_fn subdir_cb,\n \t\t\t\t void *data);\n+int for_each_loose_file_in_objdir_buf(struct strbuf *path,\n+\t\t\t\t each_loose_object_fn obj_cb,\n+\t\t\t\t each_loose_cruft_fn cruft_cb,\n+\t\t\t\t each_loose_subdir_fn subdir_cb,\n+\t\t\t\t void *data);\n \n /*\n * Iterate over loose and packed objects in both the local\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c632641..725de7f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3358,31 +3358,42 @@ static int for_each_file_in_obj_subdir(int subdir_nr,\n \treturn r;\n }\n \n-int for_each_loose_file_in_objdir(const char *path,\n+int for_each_loose_file_in_objdir_buf(struct strbuf *path,\n \t\t\t each_loose_object_fn obj_cb,\n \t\t\t each_loose_cruft_fn cruft_cb,\n \t\t\t each_loose_subdir_fn subdir_cb,\n \t\t\t void *data)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tsize_t baselen;\n+\tsize_t baselen = path->len;\n \tint r = 0;\n \tint i;\n \n-\tstrbuf_addstr(&buf, path);\n-\tstrbuf_addch(&buf, '/');\n-\tbaselen = buf.len;\n-\n \tfor (i = 0; i < 256; i++) {\n-\t\tstrbuf_addf(&buf, \"%02x\", i);\n-\t\tr = for_each_file_in_obj_subdir(i, &buf, obj_cb, cruft_cb,\n+\t\tstrbuf_addf(path, \"/%02x\", i);\n+\t\tr = for_each_file_in_obj_subdir(i, path, obj_cb, cruft_cb,\n \t\t\t\t\t\tsubdir_cb, data);\n-\t\tstrbuf_setlen(&buf, baselen);\n+\t\tstrbuf_setlen(path, baselen);\n \t\tif (r)\n \t\t\tbreak;\n \t}\n \n+\treturn r;\n+}\n+\n+int for_each_loose_file_in_objdir(const char *path,\n+\t\t\t\t each_loose_object_fn obj_cb,\n+\t\t\t\t each_loose_cruft_fn cruft_cb,\n+\t\t\t\t each_loose_subdir_fn subdir_cb,\n+\t\t\t\t void *data)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint r;\n+\n+\tstrbuf_addstr(&buf, path);\n+\tr = for_each_loose_file_in_objdir_buf(&buf, obj_cb, cruft_cb,\n+\t\t\t\t\t subdir_cb, data);\n \tstrbuf_release(&buf);\n+\n \treturn r;\n }\n \n-- \n2.3.0.rc1.287.g761fd19\n"},{"id":"255793","messageId":"20150209011538.GB21123@peff.net","threadId":"38532","inReplyTo":"20150209011159.GA21072@peff.net","subject":"[PATCH 2/2] sha1_file: fix iterating loose alternate objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-09T01:15:39Z","receivedAt":"2015-02-09T01:15:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jonathon Mah \n\nThe string in 'base' contains a path suffix to a specific object;\nwhen its value is used, the suffix must either be filled (as in\nstat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or\ncleared (as in prepare_packed_git) to avoid junk at the end.\n\n660c889e (sha1_file: add for_each iterators for loose and packed\nobjects, 2014-10-15) introduced loose_from_alt_odb(), but this did\nneither and treated 'base' as a complete path to the \"base\" object\ndirectory, instead of a pointer to the \"base\" of the full path\nstring.\n\nThe trailing path after 'base' is still initialized to NUL, hiding\nthe bug in some common cases. Additionally the descendent\nfor_each_file_in_obj_subdir() function swallows ENOENT, so an error\nonly shows if the alternate's path was last filled with a valid\nobject (where statting /path/to/existing/00/0bjectfile/00 fails).\n\nSigned-off-by: Jonathon Mah \nHelped-by: Kyle J. McKay \nSigned-off-by: Jeff King \n---\nI think the S-O-B should still stand, as the code is now a mix of our\nwork, and the tests are still Jonathon's. But let me know if you do not\nwant your name attached to this. ;)\n\nI am also happy to build it as a patch on top of the original if that is\nsimpler.\n\n sha1_file.c | 13 ++++++++++---\n t/t5304-prune.sh | 8 ++++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 725de7f..a41cc4f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3406,9 +3406,16 @@ static int loose_from_alt_odb(struct alternate_object_database *alt,\n \t\t\t void *vdata)\n {\n \tstruct loose_alt_odb_data *data = vdata;\n-\treturn for_each_loose_file_in_objdir(alt->base,\n-\t\t\t\t\t data->cb, NULL, NULL,\n-\t\t\t\t\t data->data);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint r;\n+\n+\t/* copy base not including trailing '/' */\n+\tstrbuf_add(&buf, alt->base, alt->name - alt->base - 1);\n+\tr = for_each_loose_file_in_objdir_buf(&buf,\n+\t\t\t\t\t data->cb, NULL, NULL,\n+\t\t\t\t\t data->data);\n+\tstrbuf_release(&buf);\n+\treturn r;\n }\n \n int for_each_loose_object(each_loose_object_fn cb, void *data)\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex e32e46d..0794d33 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -253,4 +253,12 @@ test_expect_success 'prune .git/shallow' '\n \ttest_path_is_missing .git/shallow\n '\n \n+test_expect_success 'prune: handle alternate object database' '\n+\ttest_create_repo A &&\n+\tgit -C A commit --allow-empty -m \"initial commit\" &&\n+\tgit clone --shared A B &&\n+\tgit -C B commit --allow-empty -m \"next commit\" &&\n+\tgit -C B prune\n+'\n+\n test_done\n-- \n2.3.0.rc1.287.g761fd19\n"},{"id":"255811","messageId":"9DC6FCD7-8C5B-464F-B47F-EC8AC37E9549@gmail.com","threadId":"38532","inReplyTo":"20150209011538.GB21123@peff.net","subject":"Re: [PATCH 2/2] sha1_file: fix iterating loose alternate objects","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-09T09:44:51Z","receivedAt":"2015-02-09T09:44:51Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Feb 8, 2015, at 17:15, Jeff King wrote:\n[...]\n> Signed-off-by: Jonathon Mah \n> Helped-by: Kyle J. McKay \n> Signed-off-by: Jeff King \n> ---\n> I think the S-O-B should still stand, as the code is now a mix of our\n> work, and the tests are still Jonathon's. But let me know if you do \n> not\n> want your name attached to this. ;)\n\nThat's fine.\n\nThis fix looks much better. :)\n\nUnfortunately I can no longer reproduce the original bug as the \nrepository that caused it is no longer in a state that triggers the \nproblem (and my backups of it are either slightly too old or slightly \ntoo new).\n\n-Kyle\n"},{"id":"255832","messageId":"xmqqpp9ic453.fsf@gitster.dls.corp.google.com","threadId":"38532","inReplyTo":"20150209005444.GA16827@peff.net","subject":"Re: [PATCH] sha1_file.c: make sure open_sha1_file does not open a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-09T18:48:40Z","receivedAt":"2015-02-09T18:48:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> However, the first thing for_each_loose_file_in_objdir is going to do is\n> stick the path into a strbuf. So perhaps the most sensible thing is to\n> just teach it to take a strbuf from the caller. I'll work up a patch.\n>\n> It looks like a1b47246 isn't even in \"next\" yet, so I'll build it\n> directly on what is already in master, dropping Jonathan's patch.\n\nThanks; looks very sensible.\n"}]}