{"thread":{"id":"40664","subject":"Bug: Segfault when doing \"git diff\"","startedAt":"2015-10-28T11:58:46Z","lastAt":"2015-10-28T17:27:58Z","messageCount":7,"participants":["Mathias L. Baumann","Victor Leschuk","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"272402","messageId":"5630B876.7080407@sociomantic.com","threadId":"40664","inReplyTo":null,"subject":"Bug: Segfault when doing \"git diff\"","fromName":"Mathias L. Baumann","fromEmail":"mathias.baumann@sociomantic.com","sentAt":"2015-10-28T11:58:46Z","receivedAt":"2015-10-28T11:58:46Z","isPatch":false,"sender":{"key":"mathias.baumann@sociomantic.com","avatar":null},"body":"Hello dear git devs,\n\nI just stumbled upon a segfault when doing just \"git diff\" in my repo.\n\nI managed to create a minimal repo setup where the bug is reproducable.\n\nThe problem seems to be a mix of having an untracked submodule and \nhaving set an alternates file for one submodule.\n\nAttached you'll find my setup that will reproduce the problem. Simply \nrun  'git diff' in bugtest1.\n\nIn case the attachment is refused, I also uploaded it here:\n\nhttp://supraverse.net/bugdemo.tar.gz\n\ncheers,\n\n     --Marenz\n"},{"id":"272403","messageId":"5630BE79.40708@gmail.com","threadId":"40664","inReplyTo":"5630B876.7080407@sociomantic.com","subject":"Re: Bug: Segfault when doing \"git diff\"","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-10-28T12:24:25Z","receivedAt":"2015-10-28T12:24:25Z","isPatch":false,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"\n\nOn 10/28/2015 02:58 PM, Mathias L. Baumann wrote:\n> Hello dear git devs,\n>\n> I just stumbled upon a segfault when doing just \"git diff\" in my repo.\n>\n> I managed to create a minimal repo setup where the bug is reproducable.\n>\n> The problem seems to be a mix of having an untracked submodule and \n> having set an alternates file for one submodule.\n>\n> Attached you'll find my setup that will reproduce the problem. Simply \n> run  'git diff' in bugtest1.\n>\n> In case the attachment is refused, I also uploaded it here:\n>\n> http://supraverse.net/bugdemo.tar.gz\n>\n> cheers,\n>\n>     --Marenz\nHello Marenz,\n\nI have just tried to reproduce segfault with the provided archive:\n\n[del@del-debian bugtest1 (master)]$ git diff\ndiff --git a/submodules/bugtest2 b/submodules/bugtest2\n--- a/submodules/bugtest2\n+++ b/submodules/bugtest2\n@@ -1 +1 @@\n-Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126\n+Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126-dirty\n\nNo segfault occured. I am using\n\ngit version 2.6.2.308.g3b8f10c\n\nCould you please specify which version of git you are using and also try \nto reproduce it with latest 2.6.2?\n\n--\nVictor\n"},{"id":"272405","messageId":"5630CF1B.9000706@sociomantic.com","threadId":"40664","inReplyTo":"5630BE79.40708@gmail.com","subject":"Re: Bug: Segfault when doing \"git diff\"","fromName":"Mathias L. Baumann","fromEmail":"mathias.baumann@sociomantic.com","sentAt":"2015-10-28T13:35:23Z","receivedAt":"2015-10-28T13:35:23Z","isPatch":false,"sender":{"key":"mathias.baumann@sociomantic.com","avatar":null},"body":"I was using the latest git version 2.6.2 already.\nI suspect it is due to a .gitconfig. This is what is probably required:\n\n➜  ~  cat .gitconfig\n[diff]\n     submodule = log\n\n\nOn 28/10/15 13:24, Victor Leschuk wrote:\n>\n>\n> On 10/28/2015 02:58 PM, Mathias L. Baumann wrote:\n>> Hello dear git devs,\n>>\n>> I just stumbled upon a segfault when doing just \"git diff\" in my repo.\n>>\n>> I managed to create a minimal repo setup where the bug is reproducable.\n>>\n>> The problem seems to be a mix of having an untracked submodule and\n>> having set an alternates file for one submodule.\n>>\n>> Attached you'll find my setup that will reproduce the problem. Simply\n>> run  'git diff' in bugtest1.\n>>\n>> In case the attachment is refused, I also uploaded it here:\n>>\n>> http://supraverse.net/bugdemo.tar.gz\n>>\n>> cheers,\n>>\n>>     --Marenz\n> Hello Marenz,\n>\n> I have just tried to reproduce segfault with the provided archive:\n>\n> [del@del-debian bugtest1 (master)]$ git diff\n> diff --git a/submodules/bugtest2 b/submodules/bugtest2\n> --- a/submodules/bugtest2\n> +++ b/submodules/bugtest2\n> @@ -1 +1 @@\n> -Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126\n> +Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126-dirty\n>\n> No segfault occured. I am using\n>\n> git version 2.6.2.308.g3b8f10c\n>\n> Could you please specify which version of git you are using and also try\n> to reproduce it with latest 2.6.2?\n>\n> --\n> Victor\n"},{"id":"272408","messageId":"5630D38A.7080301@gmail.com","threadId":"40664","inReplyTo":"5630CF1B.9000706@sociomantic.com","subject":"Re: Bug: Segfault when doing \"git diff\"","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-10-28T13:54:18Z","receivedAt":"2015-10-28T13:54:18Z","isPatch":false,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"\n\nOn 10/28/2015 04:35 PM, Mathias L. Baumann wrote:\n> I was using the latest git version 2.6.2 already.\n> I suspect it is due to a .gitconfig. This is what is probably required:\n>\n> ➜  ~  cat .gitconfig\n> [diff]\n>     submodule = log\nYep, that did the trick.\n\nThe segfault is from\n\nsha1_file.c:\n\n/* add the alternate entry */\n  *alt_odb_tail = ent; /* <===== alt_obd_tail is NULL here */\nalt_odb_tail = &(ent->next);\nent->next = NULL;\n\nWill try to take a closer look at it.\n\n--\nVictor\n"},{"id":"272407","messageId":"20151028140725.GA15304@sigill.intra.peff.net","threadId":"40664","inReplyTo":"5630CF1B.9000706@sociomantic.com","subject":"[PATCH] add_submodule_odb: initialize alt_odb list earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-28T14:07:25Z","receivedAt":"2015-10-28T14:07:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 28, 2015 at 02:35:23PM +0100, Mathias L. Baumann wrote:\n\n> I was using the latest git version 2.6.2 already.\n> I suspect it is due to a .gitconfig. This is what is probably required:\n> \n> ➜  ~  cat .gitconfig\n> [diff]\n>     submodule = log\n\nYeah, I can reproduce it easily with that. Thanks for providing the\nrepository. It takes a rather convoluted set of conditions to trigger\nthe bug. :)\n\nHere's the fix:\n\n-- >8 --\nSubject: add_submodule_odb: initialize alt_odb list earlier\n\nThe add_submodule_odb function tries to add a submodule's\nobject store as an \"alternate\". It needs the existing list\nto be initialized (from the objects/info/alternates file)\nfor two reasons:\n\n  1. We look for duplicates with the existing alternate\n     stores, but obviously this doesn't work if we haven't\n     loaded any yet.\n\n  2. We link our new entry into the list by prepending it to\n     alt_odb_list. But we do _not_ modify alt_odb_tail.\n     This variable starts as NULL, and is a signal to the\n     alt_odb code that the list has not yet been\n     initialized.\n\n     We then call read_info_alternates on the submodule (to\n     recursively load its alternates), which will try to\n     append to that tail, assuming it has been initialized.\n     This causes us to segfault if it is NULL.\n\nThis rarely comes up in practice, because we will have\ninitialized the alt_odb any time we do an object lookup. So\nyou can trigger this only when:\n\n  - you try to access a submodule (e.g., a diff with\n    diff.submodule=log)\n\n  - the access happens before any other object has been\n    accessed (e.g., because the diff is between the working\n    tree and the index)\n\n  - the submodule contains an alternates file (so we try to\n    add an entry to the NULL alt_odb_tail)\n\nTo fix this, we just need to call prepare_alt_odb at the\nstart of the function (and if we have already initialized,\nit is a noop).\n\nNote that we can remove the prepare_alt_odb call from the\nend. It is guaranteed to be a noop, since we will have\ncalled it earlier.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n submodule.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5879cfb..88af54c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -130,6 +130,7 @@ static int add_submodule_odb(const char *path)\n \t\tgoto done;\n \t}\n \t/* avoid adding it twice */\n+\tprepare_alt_odb();\n \tfor (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n \t\tif (alt_odb->name - alt_odb->base == objects_directory.len &&\n \t\t\t\t!strncmp(alt_odb->base, objects_directory.buf,\n@@ -148,7 +149,6 @@ static int add_submodule_odb(const char *path)\n \n \t/* add possible alternates from the submodule */\n \tread_info_alternates(objects_directory.buf, 0);\n-\tprepare_alt_odb();\n done:\n \tstrbuf_release(&objects_directory);\n \treturn ret;\n-- \n2.6.2.572.g6ed22dd\n"},{"id":"272413","messageId":"xmqqa8r3m2xq.fsf@gitster.mtv.corp.google.com","threadId":"40664","inReplyTo":"20151028140725.GA15304@sigill.intra.peff.net","subject":"Re: [PATCH] add_submodule_odb: initialize alt_odb list earlier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-28T15:24:17Z","receivedAt":"2015-10-28T15:24:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, I can reproduce it easily with that. Thanks for providing the\n> repository. It takes a rather convoluted set of conditions to trigger\n> the bug. :)\n>\n> Here's the fix:\n>\n> -- >8 --\n> Subject: add_submodule_odb: initialize alt_odb list earlier\n>\n> The add_submodule_odb function tries to add a submodule's\n> object store as an \"alternate\". It needs the existing list\n> to be initialized (from the objects/info/alternates file)\n> for two reasons:\n>\n>   1. We look for duplicates with the existing alternate\n>      stores, but obviously this doesn't work if we haven't\n>      loaded any yet.\n>\n>   2. We link our new entry into the list by prepending it to\n>      alt_odb_list. But we do _not_ modify alt_odb_tail.\n>      This variable starts as NULL, and is a signal to the\n>      alt_odb code that the list has not yet been\n>      initialized.\n>\n>      We then call read_info_alternates on the submodule (to\n>      recursively load its alternates), which will try to\n>      append to that tail, assuming it has been initialized.\n>      This causes us to segfault if it is NULL.\n>\n> This rarely comes up in practice, because we will have\n> initialized the alt_odb any time we do an object lookup. So\n> you can trigger this only when:\n>\n>   - you try to access a submodule (e.g., a diff with\n>     diff.submodule=log)\n>\n>   - the access happens before any other object has been\n>     accessed (e.g., because the diff is between the working\n>     tree and the index)\n>\n>   - the submodule contains an alternates file (so we try to\n>     add an entry to the NULL alt_odb_tail)\n>\n> To fix this, we just need to call prepare_alt_odb at the\n> start of the function (and if we have already initialized,\n> it is a noop).\n>\n> Note that we can remove the prepare_alt_odb call from the\n> end. It is guaranteed to be a noop, since we will have\n> called it earlier.\n\nThanks for a quick and detailed diagnosis and a fix.\n\nThe removal is correct, but even without this fix, the order of\ncalls in the original should have screamed \"bug\" loudly at us, I\nthink.  We shouldn't be reading data from alternates file without\nfirst preparing the place we read data into.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  submodule.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 5879cfb..88af54c 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -130,6 +130,7 @@ static int add_submodule_odb(const char *path)\n>  \t\tgoto done;\n>  \t}\n>  \t/* avoid adding it twice */\n> +\tprepare_alt_odb();\n>  \tfor (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n>  \t\tif (alt_odb->name - alt_odb->base == objects_directory.len &&\n>  \t\t\t\t!strncmp(alt_odb->base, objects_directory.buf,\n> @@ -148,7 +149,6 @@ static int add_submodule_odb(const char *path)\n>  \n>  \t/* add possible alternates from the submodule */\n>  \tread_info_alternates(objects_directory.buf, 0);\n> -\tprepare_alt_odb();\n>  done:\n>  \tstrbuf_release(&objects_directory);\n>  \treturn ret;\n"},{"id":"272421","messageId":"20151028172758.GA21851@sigill.intra.peff.net","threadId":"40664","inReplyTo":"xmqqa8r3m2xq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] add_submodule_odb: initialize alt_odb list earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-28T17:27:58Z","receivedAt":"2015-10-28T17:27:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 28, 2015 at 08:24:17AM -0700, Junio C Hamano wrote:\n\n> > Note that we can remove the prepare_alt_odb call from the\n> > end. It is guaranteed to be a noop, since we will have\n> > called it earlier.\n> \n> Thanks for a quick and detailed diagnosis and a fix.\n> \n> The removal is correct, but even without this fix, the order of\n> calls in the original should have screamed \"bug\" loudly at us, I\n> think.  We shouldn't be reading data from alternates file without\n> first preparing the place we read data into.\n\nYeah, I agree. I spent a long time trying to figure out if that\nprepare_alt_odb was actually doing something useful (like if it was\nneeded to somehow \"cement\" the new alt into place).\n\nBut I don't think it was.\n\nIn the majority of cases, it was a noop (we had already prepared when we\nlooked up the first object). But for other cases...\n\n  - if read_info_alternates actually did something, we segfaulted (i.e.,\n    this bug)\n\n  - otherwise, we would prepare on _top_ of what we just added to the\n    list, which was probably buggy (I didn't dig far enough to see if\n    prepare_alt_odb() would overwrite what we just added to the list).\n\nSo some pretty dark corners of the code. :)\n\n-Peff\n"}]}