{"thread":{"id":"32709","subject":"[PATCH] parse_object: clear \"parsed\" when freeing buffers","startedAt":"2013-01-23T21:25:04Z","lastAt":"2013-01-24T23:27:46Z","messageCount":9,"participants":["Jonathon Mah","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"207639","messageId":"8988071A-1DF3-463E-8AF9-AE4EA200D786@me.com","threadId":"32709","inReplyTo":null,"subject":"[PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Jonathon Mah","fromEmail":"jmah@me.com","sentAt":"2013-01-23T21:25:04Z","receivedAt":"2013-01-23T21:25:04Z","isPatch":true,"sender":{"key":"jmah@me.com","avatar":null},"body":"Add a new function \"free_object_buffer\", which marks the object as\nun-parsed and frees the buffer. Only trees and commits have buffers;\nother types are not affected. If the tree or commit buffer is already\nNULL, the \"parsed\" flag is still cleared so callers can control the free\nthemselves (index-pack.c uses this).\n\nSeveral areas of code would free buffers for object structs that\ncontained them (\"struct tree\" and \"struct commit\"), but without clearing\nthe \"parsed\" flag. parse_object would clear the flag for \"struct tree\",\nbut commits would remain in an invalid state (marked as parsed but with\na NULL buffer). Because the objects are uniqued (ccdc6037fee), the\ninvalid objects stay around and can lead to bad behavior.\n\nIn particular, running pickaxe on all refs in a repository with a cached\ntextconv could segfault. If the textconv cache ref was evaluated first\nby cmd_log_walk, a subsequent notes_cache_match_validity call would\ndereference NULL.\n\nSigned-off-by: Jonathon Mah \n---\n\nI found an old email where Jeff noted that this would be bad (yet the buffer manipulation remained).\n\n\n builtin/fsck.c | 17 ++---------------\n builtin/index-pack.c | 3 +++\n builtin/log.c | 9 +++------\n builtin/reflog.c | 3 +--\n builtin/rev-list.c | 3 +--\n http-push.c | 3 +--\n list-objects.c | 3 +--\n object.c | 25 +++++++++++++++++++++++--\n object.h | 3 +++\n reachable.c | 3 +--\n revision.c | 3 +--\n t/t4042-diff-textconv-caching.sh | 11 +++++++++++\n upload-pack.c | 3 +--\n walker.c | 5 +----\n 14 files changed, 53 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex bb9a2cd..82b3612 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -133,10 +133,7 @@ static int traverse_one_object(struct object *obj)\n \t\t\treturn 1; /* error already displayed */\n \t}\n \tresult = fsck_walk(obj, mark_object, obj);\n-\tif (tree) {\n-\t\tfree(tree->buffer);\n-\t\ttree->buffer = NULL;\n-\t}\n+\tfree_object_buffer(obj);\n \treturn result;\n }\n \n@@ -303,26 +300,16 @@ static int fsck_obj(struct object *obj)\n \tif (fsck_object(obj, check_strict, fsck_error_func))\n \t\treturn -1;\n \n-\tif (obj->type == OBJ_TREE) {\n-\t\tstruct tree *item = (struct tree *) obj;\n-\n-\t\tfree(item->buffer);\n-\t\titem->buffer = NULL;\n-\t}\n+\tfree_object_buffer(obj);\n \n \tif (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n-\n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n-\n \t\tif (!commit->parents && show_root)\n \t\t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\n \t}\n \n \tif (obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *) obj;\n-\n \t\tif (show_tags && tag->tagged) {\n \t\t\tprintf(\"tagged %s %s\", typename(tag->tagged->type), sha1_to_hex(tag->tagged->sha1));\n \t\t\tprintf(\" (%s) in %s\\n\", tag->tag, sha1_to_hex(tag->object.sha1));\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 43d364b..0eb39ae 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -750,13 +750,16 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\n \n+\t\t\t/* set buffer to NULL so it isn't freed */\n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n \t\t\t\titem->buffer = NULL;\n+\t\t\t\tfree_object_buffer(obj);\n \t\t\t}\n \t\t\tif (obj->type == OBJ_COMMIT) {\n \t\t\t\tstruct commit *commit = (struct commit *) obj;\n \t\t\t\tcommit->buffer = NULL;\n+\t\t\t\tfree_object_buffer(obj);\n \t\t\t}\n \t\t\tobj->flags |= FLAG_CHECKED;\n \t\t}\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e7b7db1..433b874 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -305,11 +305,9 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\t\t * but we didn't actually show the commit.\n \t\t\t */\n \t\t\trev->max_count++;\n-\t\tif (!rev->reflog_info) {\n+\t\tif (!rev->reflog_info)\n \t\t\t/* we allow cycles in reflog ancestry */\n-\t\t\tfree(commit->buffer);\n-\t\t\tcommit->buffer = NULL;\n-\t\t}\n+\t\t\tfree_object_buffer(&commit->object);\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t\tif (saved_nrl < rev->diffopt.needed_rename_limit)\n@@ -1399,8 +1397,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t reopen_stdout(numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"Failed to create output files\"));\n \t\tshown = log_tree_commit(&rev, commit);\n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n+\t\tfree_object_buffer(&commit->object);\n \n \t\t/* We put one extra blank line between formatted\n \t\t * patches and this flag is used by log-tree code\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex b3c9e27..c9a660f 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)\n \t\t\tcomplete = 0;\n \t\t}\n \t}\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_object_buffer(tree->buffer);\n \n \tif (complete)\n \t\ttree->object.flags |= SEEN;\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 67701be..6855892 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -170,8 +170,7 @@ static void finish_commit(struct commit *commit, void *data)\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t}\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n+\tfree_object_buffer(&commit->object);\n }\n \n static void finish_object(struct object *obj,\ndiff --git a/http-push.c b/http-push.c\nindex 8701c12..042a410 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1347,8 +1347,7 @@ static struct object_list **process_tree(struct tree *tree,\n \t\t\tbreak;\n \t\t}\n \n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_object_buffer(obj);\n \treturn p;\n }\n \ndiff --git a/list-objects.c b/list-objects.c\nindex 3dd4a96..fb4b531 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,\n \t\t\t\t cb_data);\n \t}\n \tstrbuf_setlen(base, baselen);\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_object_buffer(obj);\n }\n \n static void mark_edge_parents_uninteresting(struct commit *commit,\ndiff --git a/object.c b/object.c\nindex 4af3451..8e161f8 100644\n--- a/object.c\n+++ b/object.c\n@@ -149,8 +149,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tstruct tree *tree = lookup_tree(sha1);\n \t\tif (tree) {\n \t\t\tobj = &tree->object;\n-\t\t\tif (!tree->buffer)\n-\t\t\t\ttree->object.parsed = 0;\n \t\t\tif (!tree->object.parsed) {\n \t\t\t\tif (parse_tree_buffer(tree, buffer, size))\n \t\t\t\t\treturn NULL;\n@@ -225,6 +223,29 @@ struct object *parse_object(const unsigned char *sha1)\n \treturn NULL;\n }\n \n+void free_object_buffer(struct object *item)\n+{\n+\tif (!item)\n+\t\treturn;\n+\n+\tif (item->type == OBJ_TREE) {\n+\t\tstruct tree *tree = (struct tree *)item;\n+\t\ttree->object.parsed = 0;\n+\t\ttree->size = 0;\n+\t\tif (tree->buffer) {\n+\t\t\tfree(tree->buffer);\n+\t\t\ttree->buffer = NULL;\n+\t\t}\n+\t} else if (item->type == OBJ_COMMIT) {\n+\t\tstruct commit *commit = (struct commit *)item;\n+\t\tcommit->object.parsed = 0;\n+\t\tif (commit->buffer) {\n+\t\t\tfree(commit->buffer);\n+\t\t\tcommit->buffer = NULL;\n+\t\t}\n+\t}\n+}\n+\n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t struct object_list **list_p)\n {\ndiff --git a/object.h b/object.h\nindex 6a97b6b..cbc730c 100644\n--- a/object.h\n+++ b/object.h\n@@ -63,6 +63,9 @@ struct object *parse_object(const unsigned char *sha1);\n */\n struct object *parse_object_buffer(const unsigned char *sha1, enum object_type type, unsigned long size, void *buffer, int *eaten_p);\n \n+/** If the object's type has a buffer, it's freed and marked as un-parsed. */\n+void free_object_buffer(struct object *item);\n+\n /** Returns the object, with potentially excess memory allocated. **/\n struct object *lookup_unknown_object(const unsigned char *sha1);\n \ndiff --git a/reachable.c b/reachable.c\nindex bf79706..c29d3e0 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,\n \t\telse\n \t\t\tprocess_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);\n \t}\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_object_buffer(obj);\n }\n \n static void process_tag(struct tag *tag, struct object_array *p,\ndiff --git a/revision.c b/revision.c\nindex 95d21e6..43c5eec 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -133,8 +133,7 @@ void mark_tree_uninteresting(struct tree *tree)\n \t * We don't care about the tree any more\n \t * after it has been marked uninteresting.\n \t */\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n+\tfree_object_buffer(obj);\n }\n \n void mark_parents_uninteresting(struct commit *commit)\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex 91f8198..d7e26ca 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6142421..1feacbc 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -81,8 +81,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\tdie(\"broken output pipe\");\n \tfputc('\\n', pack_pipe);\n \tfflush(pack_pipe);\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n+\tfree_object_buffer(&commit->object);\n }\n \n static void show_object(struct object *obj,\ndiff --git a/walker.c b/walker.c\nindex be389dc..bae4876 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -56,10 +56,7 @@ static int process_tree(struct walker *walker, struct tree *tree)\n \t\tif (!obj || process(walker, obj))\n \t\t\treturn -1;\n \t}\n-\tfree(tree->buffer);\n-\ttree->buffer = NULL;\n-\ttree->size = 0;\n-\ttree->object.parsed = 0;\n+\tfree_object_buffer(&tree->object);\n \treturn 0;\n }\n \n-- \n1.8.1.1\n\n\n\n\n\nJonathon Mah\nme@JonathonMah.com\n"},{"id":"207645","messageId":"7vsj5rfspy.fsf@alter.siamese.dyndns.org","threadId":"32709","inReplyTo":"8988071A-1DF3-463E-8AF9-AE4EA200D786@me.com","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T22:19:05Z","receivedAt":"2013-01-23T22:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathon Mah writes:\n\n> Add a new function \"free_object_buffer\", which marks the object as\n> un-parsed and frees the buffer. Only trees and commits have buffers;\n> other types are not affected. If the tree or commit buffer is already\n> NULL, the \"parsed\" flag is still cleared so callers can control the free\n> themselves (index-pack.c uses this).\n>\n> Several areas of code would free buffers for object structs that\n> contained them (\"struct tree\" and \"struct commit\"), but without clearing\n> the \"parsed\" flag. parse_object would clear the flag for \"struct tree\",\n> but commits would remain in an invalid state (marked as parsed but with\n> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the\n> invalid objects stay around and can lead to bad behavior.\n>\n> In particular, running pickaxe on all refs in a repository with a cached\n> textconv could segfault. If the textconv cache ref was evaluated first\n> by cmd_log_walk, a subsequent notes_cache_match_validity call would\n> dereference NULL.\n\nConceptually this is a right thing to do, but it is unclear why this\nconversion is safe in the existing code.\n\nA codepath that used to free() and assign NULL to a buffer without\nresetting .parsed would have assumed that it can find out the parsed\nproperties of the object (e.g. .parents) without re-parsing the\nobject, and much more importantly, the modifications made by that\ncodepath will not be clobbered by later call to parse_object().\n\nIIRC, revision traversal machinery rewrites commit->parents but\ndiscards buffer when it knows that the log message is not needed\n(save_commit_buffer controls this behaviour). I do not offhand know\nif there are other instances of existing code that depend on the\ncurrent behaviour, but have you audited all the codepaths that are\naffected by this patch and codepaths that work on objects this patch\nunmarks their .parsed field will not have such a problem?\n"},{"id":"207650","messageId":"1C90CE32-F559-4E76-915E-93642F614552@me.com","threadId":"32709","inReplyTo":"7vsj5rfspy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Jonathon Mah","fromEmail":"jmah@me.com","sentAt":"2013-01-23T23:36:51Z","receivedAt":"2013-01-23T23:36:51Z","isPatch":true,"sender":{"key":"jmah@me.com","avatar":null},"body":"[Adding Jeff King to CC; I meant to copy you in the original but forgot, sorry]\n\nOn 2013-01-23, at 14:19, Junio C Hamano wrote:\n\n> Jonathon Mah writes:\n> \n>> Add a new function \"free_object_buffer\", which marks the object as\n>> un-parsed and frees the buffer. Only trees and commits have buffers;\n>> other types are not affected. If the tree or commit buffer is already\n>> NULL, the \"parsed\" flag is still cleared so callers can control the free\n>> themselves (index-pack.c uses this).\n>> \n>> Several areas of code would free buffers for object structs that\n>> contained them (\"struct tree\" and \"struct commit\"), but without clearing\n>> the \"parsed\" flag. parse_object would clear the flag for \"struct tree\",\n>> but commits would remain in an invalid state (marked as parsed but with\n>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the\n>> invalid objects stay around and can lead to bad behavior.\n>> \n>> In particular, running pickaxe on all refs in a repository with a cached\n>> textconv could segfault. If the textconv cache ref was evaluated first\n>> by cmd_log_walk, a subsequent notes_cache_match_validity call would\n>> dereference NULL.\n> \n> Conceptually this is a right thing to do, but it is unclear why this\n> conversion is safe in the existing code.\n> \n> A codepath that used to free() and assign NULL to a buffer without\n> resetting .parsed would have assumed that it can find out the parsed\n> properties of the object (e.g. .parents) without re-parsing the\n> object, and much more importantly, the modifications made by that\n> codepath will not be clobbered by later call to parse_object().\n> \n> IIRC, revision traversal machinery rewrites commit->parents but\n> discards buffer when it knows that the log message is not needed\n> (save_commit_buffer controls this behaviour). I do not offhand know\n> if there are other instances of existing code that depend on the\n> current behaviour, but have you audited all the codepaths that are\n> affected by this patch and codepaths that work on objects this patch\n> unmarks their .parsed field will not have such a problem?\n\nNo, I haven't audited the code paths (I have only the loosest familiarity with the source). Indeed, I found that clearing the 'parsed' flag in fsck.c (traverse_one_object()) is incorrect and causes test failures.\n\nWith the object cache, isn't modifying the object unsafe in general? Instead of auditing code paths, it's now necessary to audit _all_ code that uses \"struct object\", which seems infeasible.\n\nAnyway, I don't care about the implementation (Junio does that extremely well), I'm just trying to fix the segfault demonstrated in the test attached to the patch.\n\n\n\nJonathon Mah\nme@JonathonMah.com\n"},{"id":"207652","messageId":"7vfw1rfmw2.fsf@alter.siamese.dyndns.org","threadId":"32709","inReplyTo":"1C90CE32-F559-4E76-915E-93642F614552@me.com","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T00:25:01Z","receivedAt":"2013-01-24T00:25:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathon Mah writes:\n\n> No, I haven't audited the code paths (I have only the loosest\n> familiarity with the source). Indeed, I found that clearing the\n> 'parsed' flag in fsck.c (traverse_one_object()) is incorrect and\n> causes test failures.\n>\n> With the object cache, isn't modifying the object unsafe in\n> general? Instead of auditing code paths, it's now necessary to\n> audit _all_ code that uses \"struct object\", which seems\n> infeasible.\n\nThe object layer was designed with \"run one thing and one thing\nwell, and then let the _exit(2) take care of the clean-up\" model in\nmind; modifying the object, e.g. updating commit->parents list,\nin-core by revision traversal machinery is very much within the\nscope of its intended uses.\n\n> I'm just trying to fix the segfault demonstrated in the test\n> attached to the patch.\n\nCan offending readers that dereference NULL without checking if\nbuffer has been freed be updated so that they read_sha1_file(), read\nthe contents from the result returned from the function (instead of\nreading from .buffer), and free the memory when they are done?\n\nThat would be a fix that would be very much isolated and easy to\naudit.\n"},{"id":"207666","messageId":"20130124063259.GA610@sigill.intra.peff.net","threadId":"32709","inReplyTo":"7vfw1rfmw2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-24T06:33:00Z","receivedAt":"2013-01-24T06:33:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 23, 2013 at 04:25:01PM -0800, Junio C Hamano wrote:\n\n> > With the object cache, isn't modifying the object unsafe in\n> > general? Instead of auditing code paths, it's now necessary to\n> > audit _all_ code that uses \"struct object\", which seems\n> > infeasible.\n> \n> The object layer was designed with \"run one thing and one thing\n> well, and then let the _exit(2) take care of the clean-up\" model in\n> mind; modifying the object, e.g. updating commit->parents list,\n> in-core by revision traversal machinery is very much within the\n> scope of its intended uses.\n\nYeah, although I think the \"parsed\" flag really is a bit overloaded for\ncommits.\n\nMost code which uses \"struct commit\" will want the parents, timestamp,\nand tree information filled in. And so they call parse_commit to make\nsure that is the case. Afterwards, those fields are valid and the\n\"parsed\" flag is set. The buffer field may or may not be valid\nafterwards, depending on save_commit_buffer.\n\nSome other code may also care about the rest of the commit information.\nIt also calls parse_commit, but with save_commit_buffer turned on.\nHowever, that may or may not actually initialize the buffer, depending\non who has been using the object before us.\n\nIt works in practice most of the time because we only perform one \"task\"\nper invocation, and that task either keeps the commit messages around\nthe first time, or doesn't. But it is a bit of a ticking time bomb\nanytime we violate that assumption.\n\nI think it would be saner for callers who only care about ancestry info\ncall to call parse_commit, and then anybody who is about to access the\nbuffer to call a new ensure_commit_buffer function, which would make\nsure the buffer is accessible (even if save_commit_buffer is false).\n\nThen save_commit_buffer becomes just an optimization: save it for later\nduring parsing, so we don't have to do it later. That would also let us\noptimize better if we end up with a commit ancestry cache which can do\nparse_commit much cheaper than accessing the full object.\n\nFor example, my commit cache patches hook into parse_commit, but they\ncan only do so safely when save_commit_buffer is false. So git-rev-list\nbenefits, but git-log does not. However, if we knew that log would\nlazily load the commit data when needed, we could use the cache\nthere, too. It would not be a win if you are showing every commit\nanyway, but if you have limiting that does not depend on the commit\nobject itself (e.g., path limiting in the tree), you could avoid loading\nsome commits entirely.\n\n> > I'm just trying to fix the segfault demonstrated in the test\n> > attached to the patch.\n> \n> Can offending readers that dereference NULL without checking if\n> buffer has been freed be updated so that they read_sha1_file(), read\n> the contents from the result returned from the function (instead of\n> reading from .buffer), and free the memory when they are done?\n\nLooks like builtin/blame.c:get_commit_info does this already, although\nit leaves the buffer attached to the commit and does not free it.\n\nThe ensure_commit_buffer function could look something like:\n\n int ensure_commit_buffer(struct commit *item)\n {\n enum object_type type;\n unsigned long size;\n\n if (!item)\n return -1;\n if (!item->object.parsed)\n return parse_commit(item);\n if (item->buffer)\n return 0;\n\n item->buffer = read_sha1_file(item->object.sha1, &type, &size);\n if (!item->buffer)\n return error(\"Could not read %s\",\n sha1_to_hex(item->object.sha1);\n return 0;\n }\n\nand blame could replace its in-line processing with that. It does leave\nopen the question of whether callers should free() the result. But that\nwould be up to each user of the object (and it would be an optimization\nissue, not a correctness issue, as long as each user called\nensure_commit_buffer before accessing it).\n\nBut the first step there would be to audit all of the accesses of\ncommit->buffer to make sure that they call ensure_commit_buffer\n(presumably they already call parse_commit).\n\n-Peff\n"},{"id":"207669","messageId":"20130124070715.GD610@sigill.intra.peff.net","threadId":"32709","inReplyTo":"8988071A-1DF3-463E-8AF9-AE4EA200D786@me.com","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-24T07:07:15Z","receivedAt":"2013-01-24T07:07:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:\n\n> Several areas of code would free buffers for object structs that\n> contained them (\"struct tree\" and \"struct commit\"), but without clearing\n> the \"parsed\" flag. parse_object would clear the flag for \"struct tree\",\n> but commits would remain in an invalid state (marked as parsed but with\n> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the\n> invalid objects stay around and can lead to bad behavior.\n> \n> In particular, running pickaxe on all refs in a repository with a cached\n> textconv could segfault. If the textconv cache ref was evaluated first\n> by cmd_log_walk, a subsequent notes_cache_match_validity call would\n> dereference NULL.\n\nJust to be sure I understand, what is going on is something like this?\n\n Log reads all refs, including notes. After showing the notes commit,\n log frees the buffer to save memory. Later, doing the diff on a\n commit causes us to use the notes_cache mechanism. That will look at\n the commit at the tip of the notes ref, which log has already\n processed. It will call parse_commit, but that will do nothing, as the\n parsed flag is already set, but the commit buffer remains NULL.\n\nI wonder if this could be the source of the segfault here, too:\n\n http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355\n\nIf you have refs pointing to both a submodule and its superproject, a\n\"log --all --submodule -p\" might process the submodule's commits first,\nfree their buffers, and then want to show them again as part of the\nsubmodule diff. This code in builtin/log.c:\n\n if (!rev->reflog_info) {\n /* we allow cycles in reflog ancestry */\n free(commit->buffer);\n commit->buffer = NULL;\n }\n\nassumes we will only ever look at a commit once (except for\nreflog-walking), but submodule diff violates that.\n\n-Peff\n"},{"id":"207694","messageId":"7vzjzycyne.fsf@alter.siamese.dyndns.org","threadId":"32709","inReplyTo":"20130124063259.GA610@sigill.intra.peff.net","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T16:51:33Z","receivedAt":"2013-01-24T16:51:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The ensure_commit_buffer function could look something like:\n>\n> int ensure_commit_buffer(struct commit *item)\n> {\n> enum object_type type;\n> unsigned long size;\n>\n> if (!item)\n> return -1;\n> if (!item->object.parsed)\n> return parse_commit(item);\n> if (item->buffer)\n> return 0;\n>\n> item->buffer = read_sha1_file(item->object.sha1, &type, &size);\n> if (!item->buffer)\n> return error(\"Could not read %s\",\n> sha1_to_hex(item->object.sha1);\n> return 0;\n> }\n\nThe more important issue is when to release them.\n\nOnly the subcommands that know that what they are doing do not need\nto access commit->buffer and they operate solely on parsed data are\nsuppposed to flip the save_commit_buffer bit off to reclaim the\nmemory early, so new callers that violate that assumption may want\nto call ensure_*, but if they did so when somebody cares about the\nmemory pressure enough to turn the bit off need to free the buffer\nthemselves after they use it. Otherwise the resulting program may\nstill be \"correct\" but we will end up keeping buffer that nobody\nwill use, just in case it is asked again.\n"},{"id":"207720","messageId":"6B8655CE-5CC2-4228-B49D-21A30622D1B6@me.com","threadId":"32709","inReplyTo":"20130124070715.GD610@sigill.intra.peff.net","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Jonathon Mah","fromEmail":"jmah@me.com","sentAt":"2013-01-24T21:16:29Z","receivedAt":"2013-01-24T21:16:29Z","isPatch":true,"sender":{"key":"jmah@me.com","avatar":null},"body":"On 2013-01-23, at 23:07, Jeff King wrote:\n\n> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:\n> \n>> Several areas of code would free buffers for object structs that\n>> contained them (\"struct tree\" and \"struct commit\"), but without clearing\n>> the \"parsed\" flag. parse_object would clear the flag for \"struct tree\",\n>> but commits would remain in an invalid state (marked as parsed but with\n>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the\n>> invalid objects stay around and can lead to bad behavior.\n>> \n>> In particular, running pickaxe on all refs in a repository with a cached\n>> textconv could segfault. If the textconv cache ref was evaluated first\n>> by cmd_log_walk, a subsequent notes_cache_match_validity call would\n>> dereference NULL.\n> \n> Just to be sure I understand, what is going on is something like this?\n> \n> Log reads all refs, including notes. After showing the notes commit,\n> log frees the buffer to save memory. Later, doing the diff on a\n> commit causes us to use the notes_cache mechanism. That will look at\n> the commit at the tip of the notes ref, which log has already\n> processed. It will call parse_commit, but that will do nothing, as the\n> parsed flag is already set, but the commit buffer remains NULL.\n> \n> I wonder if this could be the source of the segfault here, too:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355\n\nIndeed, that description matches what I see. The other segfault seems to be in the same place too.\n\nThe segfault hits with the following stack (optimization off):\n\n0 git-log parse_commit_header + 39 (pretty.c:738)\n1 git-log format_commit_one + 3102 (pretty.c:1138)\n2 git-log format_commit_item + 177 (pretty.c:1203)\n3 git-log strbuf_expand + 172 (strbuf.c:247)\n4 git-log format_commit_message + 196 (pretty.c:1263)\n5 git-log notes_cache_match_validity + 215 (notes-cache.c:22)\n6 git-log notes_cache_init + 212 (notes-cache.c:41)\n7 git-log userdiff_get_textconv + 176 (userdiff.c:301)\n8 git-log get_textconv + 66 (diff.c:2233)\n9 git-log has_changes + 53 (diffcore-pickaxe.c:205)\n10 git-log pickaxe + 299 (diffcore-pickaxe.c:40)\n11 git-log diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)\n12 git-log diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)\n13 git-log diffcore_std + 162 (diff.c:4673)\n14 git-log log_tree_diff_flush + 43 (log-tree.c:721)\n15 git-log log_tree_diff + 566 (log-tree.c:826)\n16 git-log log_tree_commit + 63 (log-tree.c:847)\n17 git-log cmd_log_walk + 172 (log.c:301)\n18 git-log cmd_log + 186 (log.c:568)\n19 git-log run_builtin + 475 (git.c:273)\n20 git-log handle_internal_command + 199 (git.c:434)\n21 git-log main + 149 (git.c:523)\n22 libdyld.dylib start + 1\n\ncommit->message was freed and nulled earlier in a call to cmd_log_walk. This test reproduces it reliably on my machine:\n\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex 91f8198..d7e26ca 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n\n\n\nJonathon Mah\nme@JonathonMah.com\n"},{"id":"207734","messageId":"04B00BA5-E97F-493C-9AFE-012C1688073C@me.com","threadId":"32709","inReplyTo":"20130124070715.GD610@sigill.intra.peff.net","subject":"Re: [PATCH] parse_object: clear \"parsed\" when freeing buffers","fromName":"Jonathon Mah","fromEmail":"jmah@me.com","sentAt":"2013-01-24T23:27:46Z","receivedAt":"2013-01-24T23:27:46Z","isPatch":true,"sender":{"key":"jmah@me.com","avatar":null},"body":"On 2013-01-23, at 23:07, Jeff King wrote:\n\n> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:\n> \n>> Several areas of code would free buffers for object structs that\n>> contained them (\"struct tree\" and \"struct commit\"), but without clearing\n>> the \"parsed\" flag. parse_object would clear the flag for \"struct tree\",\n>> but commits would remain in an invalid state (marked as parsed but with\n>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the\n>> invalid objects stay around and can lead to bad behavior.\n>> \n>> In particular, running pickaxe on all refs in a repository with a cached\n>> textconv could segfault. If the textconv cache ref was evaluated first\n>> by cmd_log_walk, a subsequent notes_cache_match_validity call would\n>> dereference NULL.\n> \n> Just to be sure I understand, what is going on is something like this?\n> \n> Log reads all refs, including notes. After showing the notes commit,\n> log frees the buffer to save memory. Later, doing the diff on a\n> commit causes us to use the notes_cache mechanism. That will look at\n> the commit at the tip of the notes ref, which log has already\n> processed. It will call parse_commit, but that will do nothing, as the\n> parsed flag is already set, but the commit buffer remains NULL.\n> \n> I wonder if this could be the source of the segfault here, too:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355\n\nIndeed, that description matches what I see. The other segfault seems to be in the same place too.\n\nThe segfault hits with the following stack (optimization off):\n\n0 git-log parse_commit_header + 39 (pretty.c:738)\n1 git-log format_commit_one + 3102 (pretty.c:1138)\n2 git-log format_commit_item + 177 (pretty.c:1203)\n3 git-log strbuf_expand + 172 (strbuf.c:247)\n4 git-log format_commit_message + 196 (pretty.c:1263)\n5 git-log notes_cache_match_validity + 215 (notes-cache.c:22)\n6 git-log notes_cache_init + 212 (notes-cache.c:41)\n7 git-log userdiff_get_textconv + 176 (userdiff.c:301)\n8 git-log get_textconv + 66 (diff.c:2233)\n9 git-log has_changes + 53 (diffcore-pickaxe.c:205)\n10 git-log pickaxe + 299 (diffcore-pickaxe.c:40)\n11 git-log diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)\n12 git-log diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)\n13 git-log diffcore_std + 162 (diff.c:4673)\n14 git-log log_tree_diff_flush + 43 (log-tree.c:721)\n15 git-log log_tree_diff + 566 (log-tree.c:826)\n16 git-log log_tree_commit + 63 (log-tree.c:847)\n17 git-log cmd_log_walk + 172 (log.c:301)\n18 git-log cmd_log + 186 (log.c:568)\n19 git-log run_builtin + 475 (git.c:273)\n20 git-log handle_internal_command + 199 (git.c:434)\n21 git-log main + 149 (git.c:523)\n22 libdyld.dylib start + 1\n\ncommit->message was freed and nulled earlier in a call to cmd_log_walk. This test reproduces it reliably on my machine:\n\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex 91f8198..d7e26ca 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '\n\ttest_cmp expect actual\n'\n\n+cat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\ntest_done\n\n\n\n\nJonathon Mah\nme@JonathonMah.com\n"}]}