[PATCH] builtin/log.c: fixup format-patch --base segfault

[PATCH] builtin/log.c: fixup format-patch --base segfault

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Xiaolong,

When you next re-roll your 'xy/format-patch-base' branch could you
please squash this (or something like it) into the relevant patch.
(commit 50ff6afd, "format-patch: add '--base' option to record base
tree info", 31-03-2016).

The pu branch, for me, fails a shed load of tests in the following:

    t3301-notes.sh
    t3901-i18n-patch.sh
    t4014-format-patch.sh
    t4021-format-patch-numbered.sh
    t4028-format-patch-mime-headers.sh
    t4030-diff-textconv.sh
    t4036-format-patch-signer-mime.sh
    t4052-stat-output.sh
    t4122-apply-symlink-inside.sh
    t4150-am.sh
    t4151-am-abort.sh
    t4152-am-subjects.sh
    t4255-am-submodule.sh
    t7400-submodule-basic.sh
    t7512-status-help.sh
    t9001-send-email.sh

Looking at the first failure, the cause was a segfault while running
git-format-patch. A quick trip to the debugger showed that the segfault
was in print_bases(). Furthermore, the contents of the bases structure
passed in looked very dodgy (bases->nr_patch_id was 32767 and bases->patch_id[0]
was 0xc). Indeed, it looked like it had not been initialized ...

[NOTE: t6038-merge-text-auto.sh also fails for me, but it has nothing
to do with your patch series. ;-)]

This patch was just a quick fix, you may chose a different approach to
fix the problem (eg don't call print_bases() unconditionally ...).

ATB,
Ramsay Jones

 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 48c74f5..fed0f99 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1625,8 +1625,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		signature = strbuf_detach(&buf, NULL);
 	}
 
+	memset(&bases, 0, sizeof(bases));
 	if (base_commit || config_base_commit) {
-		memset(&bases, 0, sizeof(bases));
 		reset_revision_walk();
 		prepare_bases(&bases, base_commit, list, nr);
 	}
-- 
2.8.0

Re: [PATCH] builtin/log.c: fixup format-patch --base segfault

[Ye Xiaolong]

On Sun, Apr 03, 2016 at 02:19:53AM +0100, Ramsay Jones wrote:
>
>Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>---
>
>Hi Xiaolong,
>
>When you next re-roll your 'xy/format-patch-base' branch could you
>please squash this (or something like it) into the relevant patch.
>(commit 50ff6afd, "format-patch: add '--base' option to record base
>tree info", 31-03-2016).
>
>The pu branch, for me, fails a shed load of tests in the following:
>
>    t3301-notes.sh
>    t3901-i18n-patch.sh
>    t4014-format-patch.sh
>    t4021-format-patch-numbered.sh
>    t4028-format-patch-mime-headers.sh
>    t4030-diff-textconv.sh
>    t4036-format-patch-signer-mime.sh
>    t4052-stat-output.sh
>    t4122-apply-symlink-inside.sh
>    t4150-am.sh
>    t4151-am-abort.sh
>    t4152-am-subjects.sh
>    t4255-am-submodule.sh
>    t7400-submodule-basic.sh
>    t7512-status-help.sh
>    t9001-send-email.sh
>
>Looking at the first failure, the cause was a segfault while running
>git-format-patch. A quick trip to the debugger showed that the segfault
>was in print_bases(). Furthermore, the contents of the bases structure
>passed in looked very dodgy (bases->nr_patch_id was 32767 and bases->patch_id[0]
>was 0xc). Indeed, it looked like it had not been initialized ...
>
>[NOTE: t6038-merge-text-auto.sh also fails for me, but it has nothing
>to do with your patch series. ;-)]
>
>This patch was just a quick fix, you may chose a different approach to
>fix the problem (eg don't call print_bases() unconditionally ...).

Thanks for the report, I will squash this to avoid segfault.

Best Regards,
Xiaolong.
>
>ATB,
>Ramsay Jones
>
> builtin/log.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/builtin/log.c b/builtin/log.c
>index 48c74f5..fed0f99 100644
>--- a/builtin/log.c
>+++ b/builtin/log.c
>@@ -1625,8 +1625,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> 		signature = strbuf_detach(&buf, NULL);
> 	}
> 
>+	memset(&bases, 0, sizeof(bases));
> 	if (base_commit || config_base_commit) {
>-		memset(&bases, 0, sizeof(bases));
> 		reset_revision_walk();
> 		prepare_bases(&bases, base_commit, list, nr);
> 	}
>-- 
>2.8.0