{"thread":{"id":"36703","subject":"format-patch crashes with a huge patchset","startedAt":"2014-05-19T19:35:56Z","lastAt":"2014-05-20T17:41:58Z","messageCount":3,"participants":["Michael S. Tsirkin","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"242198","messageId":"20140519193556.GA987@redhat.com","threadId":"36703","inReplyTo":null,"subject":"format-patch crashes with a huge patchset","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-05-19T19:35:56Z","receivedAt":"2014-05-19T19:35:56Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"I tried to fump the whole history of qemu with format-patch.\nIt crashes both with v2.0.0-rc2-21-g6087111\nand with git 1.8.3.1:\n\n~/opt/libexec/git-core/git-format-patch --follow -o patches/all\ne63c3dc74bfb90e4522d075d0d5a7600c5145745..\n\nBacktrace:\n\n\nProgram received signal SIGSEGV, Segmentation fault.\n0x0814d9d5 in try_to_follow_renames (opt=0xffffc8e4,\nbase=base@entry=0x816e4fe \"\", t2=0xffffbdf0, t1=0xffffbddc) at\ntree-diff.c:227\n227 diff_opts.single_follow = opt->pathspec.items[0].match;\nMissing separate debuginfos, use: debuginfo-install\nopenssl-libs-1.0.1e-37.fc19.1.i686\n(gdb) p opt\n$1 = (struct diff_options *) 0xffffc8e4\n(gdb) where\n#0 0x0814d9d5 in try_to_follow_renames (opt=0xffffc8e4,\nbase=base@entry=0x816e4fe \"\", t2=0xffffbdf0, t1=0xffffbddc) at\ntree-diff.c:227\n#1 diff_tree_sha1 (old=0x97469b4\n\"\\372\\022\\366\\336k\\345\\236\\362\\062K\\021\\236\\300\\227\\036\\302\\217\\251\\202f\", \n new=new@entry=0x9746994 \"$\\305H\\250)\\237\\203\\266ya\\311W\\n\\274\n\\n\\027^*\\221\", base=base@entry=0x816e4fe \"\", opt=opt@entry=0xffffc8e4)\n at tree-diff.c:305\n#2 0x080fb83d in log_tree_diff (log=0xffffbf28, commit=0x9734730,\nopt=0xffffc618) at log-tree.c:780\n#3 log_tree_commit (opt=opt@entry=0xffffc618,\ncommit=commit@entry=0x9734730) at log-tree.c:810\n#4 0x08088406 in cmd_format_patch (argc=,\nargv=0xffffccc4, prefix=0x0) at builtin/log.c:1510\n#5 0x0804c666 in run_builtin (argv=0xffffccc4, argc=5, p=0x81cb524\n) at git.c:314\n#6 handle_builtin (argc=5, argv=0xffffccc4) at git.c:487\n#7 0x0804bc22 in main (argc=5, av=0xffffccc4) at git.c:584\n(gdb) p opt->pathspec.items\n$2 = (struct pathspec_item *) 0x0\n\n\nDid not debug further: could be related to the fact\nswap is disabled on my box, so attempts to allocate\nhuge amounts of RAM might fail.\n\nStill should not segv I think ...\n\n-- \nMST\n"},{"id":"242227","messageId":"20140520064920.GB5222@sigill.intra.peff.net","threadId":"36703","inReplyTo":"20140519193556.GA987@redhat.com","subject":"Re: format-patch crashes with a huge patchset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-20T06:49:20Z","receivedAt":"2014-05-20T06:49:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 19, 2014 at 10:35:56PM +0300, Michael S. Tsirkin wrote:\n\n> I tried to fump the whole history of qemu with format-patch.\n> It crashes both with v2.0.0-rc2-21-g6087111\n> and with git 1.8.3.1:\n> \n> ~/opt/libexec/git-core/git-format-patch --follow -o patches/all\n> e63c3dc74bfb90e4522d075d0d5a7600c5145745..\n\nYou can't use \"--follow\" without specifying a single pathspec. Running\n\"git log --follow\" notices and blocks this, but format-patch doesn't\n(and segfaults later when the follow code assumes there is a pathspec).\n\nI think we need:\n\n-- >8 --\nSubject: move \"--follow needs one pathspec\" rule to diff_setup_done\n\nBecause of the way \"--follow\" is implemented, we must have\nexactly one pathspec. \"git log\" enforces this restriction,\nbut other users of the revision traversal code do not. For\nexample, \"git format-patch --follow\" will segfault during\ntry_to_follow_renames, as we have no pathspecs at all.\n\nWe can push this check down into diff_setup_done, which is\nprobably a better place anyway. It is the diff code that\nintroduces this restriction, so other parts of the code\nshould not need to care themselves.\n\nReported-by: \"Michael S. Tsirkin\" \nSigned-off-by: Jeff King \n---\nI didn't include a test, as I don't think the current behavior is what\nwe want in the long run. That is, it would be nice if eventually\n--follow learned to be more flexible. Any test for this would\nnecessarily be looking for the opposite of that behavior. But maybe it's\nworth it to just have in the meantime, and anyone who works on --follow\ncan rip out the test.\n\n builtin/log.c | 8 ++------\n diff.c | 3 +++\n 2 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..3b6a6bb 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -158,13 +158,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (rev->show_notes)\n \t\tinit_display_notes(&rev->notes_opt);\n \n-\tif (rev->diffopt.pickaxe || rev->diffopt.filter)\n+\tif (rev->diffopt.pickaxe || rev->diffopt.filter ||\n+\t DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES))\n \t\trev->always_show_header = 0;\n-\tif (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {\n-\t\trev->always_show_header = 0;\n-\t\tif (rev->diffopt.pathspec.nr != 1)\n-\t\t\tusage(\"git logs can only follow renames on one pathname at a time\");\n-\t}\n \n \tif (source)\n \t\trev->show_source = 1;\ndiff --git a/diff.c b/diff.c\nindex f72769a..68bb8c5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3325,6 +3325,9 @@ void diff_setup_done(struct diff_options *options)\n \t}\n \n \toptions->diff_path_counter = 0;\n+\n+\tif (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)\n+\t\tdie(_(\"--follow requires exactly one pathspec\"));\n }\n \n static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242291","messageId":"xmqqa9ac1h5l.fsf@gitster.dls.corp.google.com","threadId":"36703","inReplyTo":"20140520064920.GB5222@sigill.intra.peff.net","subject":"Re: format-patch crashes with a huge patchset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-20T17:41:58Z","receivedAt":"2014-05-20T17:41:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, May 19, 2014 at 10:35:56PM +0300, Michael S. Tsirkin wrote:\n>\n>> I tried to fump the whole history of qemu with format-patch.\n>> It crashes both with v2.0.0-rc2-21-g6087111\n>> and with git 1.8.3.1:\n>> \n>> ~/opt/libexec/git-core/git-format-patch --follow -o patches/all\n>> e63c3dc74bfb90e4522d075d0d5a7600c5145745..\n>\n> You can't use \"--follow\" without specifying a single pathspec. Running\n> \"git log --follow\" notices and blocks this, but format-patch doesn't\n> (and segfaults later when the follow code assumes there is a pathspec).\n>\n> I think we need:\n\nLooks sensible. Intrestingly enough, this dates all the way back to\nthe original 750f7b66 (Finally implement \"git log --follow\",\n2007-06-19).\n\nRe-reading the log message of that commit was amusing for other\nreasons, by the way (the most interesting part comes after \"One\nthing to look out for:\" and especially \"Put another way:\").\n\n> -- >8 --\n> Subject: move \"--follow needs one pathspec\" rule to diff_setup_done\n>\n> Because of the way \"--follow\" is implemented, we must have\n> exactly one pathspec. \"git log\" enforces this restriction,\n> but other users of the revision traversal code do not. For\n> example, \"git format-patch --follow\" will segfault during\n> try_to_follow_renames, as we have no pathspecs at all.\n>\n> We can push this check down into diff_setup_done, which is\n> probably a better place anyway. It is the diff code that\n> introduces this restriction, so other parts of the code\n> should not need to care themselves.\n>\n> Reported-by: \"Michael S. Tsirkin\" \n> Signed-off-by: Jeff King \n> ---\n> I didn't include a test, as I don't think the current behavior is what\n> we want in the long run. That is, it would be nice if eventually\n> --follow learned to be more flexible. Any test for this would\n> necessarily be looking for the opposite of that behavior. But maybe it's\n> worth it to just have in the meantime, and anyone who works on --follow\n> can rip out the test.\n>\n> builtin/log.c | 8 ++------\n> diff.c | 3 +++\n> 2 files changed, 5 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 39e8836..3b6a6bb 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -158,13 +158,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n> \tif (rev->show_notes)\n> \t\tinit_display_notes(&rev->notes_opt);\n> \n> -\tif (rev->diffopt.pickaxe || rev->diffopt.filter)\n> +\tif (rev->diffopt.pickaxe || rev->diffopt.filter ||\n> +\t DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES))\n> \t\trev->always_show_header = 0;\n> -\tif (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {\n> -\t\trev->always_show_header = 0;\n> -\t\tif (rev->diffopt.pathspec.nr != 1)\n> -\t\t\tusage(\"git logs can only follow renames on one pathname at a time\");\n> -\t}\n> \n> \tif (source)\n> \t\trev->show_source = 1;\n> diff --git a/diff.c b/diff.c\n> index f72769a..68bb8c5 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3325,6 +3325,9 @@ void diff_setup_done(struct diff_options *options)\n> \t}\n> \n> \toptions->diff_path_counter = 0;\n> +\n> +\tif (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)\n> +\t\tdie(_(\"--follow requires exactly one pathspec\"));\n> }\n> \n> static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n"}]}