Re: [bug] Segfault in git commit when a hook has a broken shebang

> -- >8 --
> Subject: [PATCH] hook API: don't segfault on strbuf_addf() to NULL "out"
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Fix a logic error in a082345372e (hook API: fix v2.36.0 regression:
> hooks should be connected to a TTY, 2022-06-07). When it started using
> the "ungroup" API added in fd3aaf53f71 (run-command: add an "ungroup"
> option to run_process_parallel(), 2022-06-07) it should have made the
> same sort of change that fd3aaf53f71 itself made in
> "t/helper/test-run-command.c".
>
> The correct way to emit this "Couldn't start" output with "ungroup"
> would be:
>
>         fprintf(stderr, _("Couldn't start hook '%s'\n"), hook_path);
>
> But we should instead remove the emitting of this output. As the added
> test shows we already emit output when we can't run the child. The
> "cannot run" output here is emitted by run-command.c's
> child_err_spew().

>
> So the addition of the "Couldn't start hook" output here in
> 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) was always
> redundant. For the pre-commit hook we'll now emit exactly the same
> output as we did before f443246b9f2 (commit: convert
> {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22) (and
> likewise for others).
>
> We could at this point add this to the pick_next_hook() callbacks in
> hook.c:
>
>         assert(!out);
>         assert(!*pp_task_cb);
>
> And this to notify_start_failure() and notify_hook_finished() (in the
> latter case the parameter is called "pp_task_cp"):
>
>         assert(!out);
>         assert(!pp_task_cb);
>
> But let's leave any such instrumentation for some eventual cleanup of
> the "ungroup" API.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

> ---
>  hook.c          |  7 -------
>  t/t1800-hook.sh | 15 +++++++++++++++
>  2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/hook.c b/hook.c
> index d113ee7faae..a493939a4fc 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -62,9 +62,6 @@ static int pick_next_hook(struct child_process *cp,
>         strvec_push(&cp->args, hook_path);
>         strvec_pushv(&cp->args, hook_cb->options->args.v);
>
> -       /* Provide context for errors if necessary */
> -       *pp_task_cb = (char *)hook_path;
> -
>         /*
>          * This pick_next_hook() will be called again, we're only
>          * running one hook, so indicate that no more work will be
> @@ -80,13 +77,9 @@ static int notify_start_failure(struct strbuf *out,
>                                 void *pp_task_cp)
>  {
>         struct hook_cb_data *hook_cb = pp_cb;
> -       const char *hook_path = pp_task_cp;
>
>         hook_cb->rc |= 1;
>
> -       strbuf_addf(out, _("Couldn't start hook '%s'\n"),
> -                   hook_path);
> -
>         return 1;
>  }
>
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 210f4298872..75f72ffbf27 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -151,4 +151,19 @@ test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' '
>         test_hook_tty commit -m"B.new"
>  '
>
> +test_expect_success 'git hook run a hook with a bad shebang' '
> +       test_when_finished "rm -rf bad-hooks" &&
> +       mkdir bad-hooks &&
> +       write_script bad-hooks/test-hook "/bad/path/no/spaces" </dev/null &&
> +       cat >expect <<-\EOF &&
> +       fatal: cannot run bad-hooks/test-hook: ...
> +       EOF
> +       test_expect_code 1 git \
> +               -c core.hooksPath=bad-hooks \
> +               hook run test-hook >out 2>actual.raw &&
> +       test_must_be_empty out &&
> +       sed -e "s/test-hook: .*/test-hook: .../" <actual.raw >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --