git-commit-vandalism/hook.c

175 lines
3.6 KiB
C
Raw Normal View History

#include "cache.h"
#include "hook.h"
#include "run-command.h"
#include "config.h"
const char *find_hook(const char *name)
{
static struct strbuf path = STRBUF_INIT;
strbuf_reset(&path);
strbuf_git_path(&path, "hooks/%s", name);
if (access(path.buf, X_OK) < 0) {
int err = errno;
#ifdef STRIP_EXTENSION
strbuf_addstr(&path, STRIP_EXTENSION);
if (access(path.buf, X_OK) >= 0)
return path.buf;
if (errno == EACCES)
err = errno;
#endif
if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
static struct string_list advise_given = STRING_LIST_INIT_DUP;
if (!string_list_lookup(&advise_given, name)) {
string_list_insert(&advise_given, name);
advise(_("The '%s' hook was ignored because "
"it's not set as executable.\n"
"You can disable this warning with "
"`git config advice.ignoredHook false`."),
path.buf);
}
}
return NULL;
}
return path.buf;
}
int hook_exists(const char *name)
{
return !!find_hook(name);
}
static int pick_next_hook(struct child_process *cp,
struct strbuf *out,
void *pp_cb,
void **pp_task_cb)
{
struct hook_cb_data *hook_cb = pp_cb;
const char *hook_path = hook_cb->hook_path;
if (!hook_path)
return 0;
cp->no_stdin = 1;
strvec_pushv(&cp->env, hook_cb->options->env.v);
cp->stdout_to_stderr = 1;
cp->trace2_hook_name = hook_cb->hook_name;
cp->dir = hook_cb->options->dir;
strvec_push(&cp->args, hook_path);
strvec_pushv(&cp->args, hook_cb->options->args.v);
/*
* This pick_next_hook() will be called again, we're only
* running one hook, so indicate that no more work will be
* done.
*/
hook_cb->hook_path = NULL;
return 1;
}
static int notify_start_failure(struct strbuf *out,
void *pp_cb,
void *pp_task_cp)
{
struct hook_cb_data *hook_cb = pp_cb;
hook_cb->rc |= 1;
return 1;
}
static int notify_hook_finished(int result,
struct strbuf *out,
void *pp_cb,
void *pp_task_cb)
{
struct hook_cb_data *hook_cb = pp_cb;
hooks: fix an obscure TOCTOU "did we just run a hook?" race Fix a Time-of-check to time-of-use (TOCTOU) race in code added in 680ee550d72 (commit: skip discarding the index if there is no pre-commit hook, 2017-08-14). This obscure race condition can occur if we e.g. ran the "pre-commit" hook and it modified the index, but hook_exists() returns false later on (e.g., because the hook itself went away, the directory became unreadable, etc.). Then we won't call discard_cache() when we should have. The race condition itself probably doesn't matter, and users would have been unlikely to run into it in practice. This problem has been noted on-list when 680ee550d72 was discussed[1], but had not been fixed. This change is mainly intended to improve the readability of the code involved, and to make reasoning about it more straightforward. It wasn't as obvious what we were trying to do here, but by having an "invoked_hook" it's clearer that e.g. our discard_cache() is happening because of the earlier hook execution. Let's also change this for the push-to-checkout hook. Now instead of checking if the hook exists and either doing a push to checkout or a push to deploy we'll always attempt a push to checkout. If the hook doesn't exist we'll fall back on push to deploy. The same behavior as before, without the TOCTOU race. See 0855331941b (receive-pack: support push-to-checkout hook, 2014-12-01) for the introduction of the previous behavior. This leaves uses of hook_exists() in two places that matter. The "reference-transaction" check in refs.c, see 67541597670 (refs: implement reference transaction hook, 2020-06-19), and the "prepare-commit-msg" hook, see 66618a50f9c (sequencer: run 'prepare-commit-msg' hook, 2018-01-24). In both of those cases we're saving ourselves CPU time by not preparing data for the hook that we'll then do nothing with if we don't have the hook. So using this "invoked_hook" pattern doesn't make sense in those cases. The "reference-transaction" and "prepare-commit-msg" hook also aren't racy. In those cases we'll skip the hook runs if we race with a new hook being added, whereas in the TOCTOU races being fixed here we were incorrectly skipping the required post-hook logic. 1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-03-07 13:33:46 +01:00
struct run_hooks_opt *opt = hook_cb->options;
hook_cb->rc |= result;
hooks: fix an obscure TOCTOU "did we just run a hook?" race Fix a Time-of-check to time-of-use (TOCTOU) race in code added in 680ee550d72 (commit: skip discarding the index if there is no pre-commit hook, 2017-08-14). This obscure race condition can occur if we e.g. ran the "pre-commit" hook and it modified the index, but hook_exists() returns false later on (e.g., because the hook itself went away, the directory became unreadable, etc.). Then we won't call discard_cache() when we should have. The race condition itself probably doesn't matter, and users would have been unlikely to run into it in practice. This problem has been noted on-list when 680ee550d72 was discussed[1], but had not been fixed. This change is mainly intended to improve the readability of the code involved, and to make reasoning about it more straightforward. It wasn't as obvious what we were trying to do here, but by having an "invoked_hook" it's clearer that e.g. our discard_cache() is happening because of the earlier hook execution. Let's also change this for the push-to-checkout hook. Now instead of checking if the hook exists and either doing a push to checkout or a push to deploy we'll always attempt a push to checkout. If the hook doesn't exist we'll fall back on push to deploy. The same behavior as before, without the TOCTOU race. See 0855331941b (receive-pack: support push-to-checkout hook, 2014-12-01) for the introduction of the previous behavior. This leaves uses of hook_exists() in two places that matter. The "reference-transaction" check in refs.c, see 67541597670 (refs: implement reference transaction hook, 2020-06-19), and the "prepare-commit-msg" hook, see 66618a50f9c (sequencer: run 'prepare-commit-msg' hook, 2018-01-24). In both of those cases we're saving ourselves CPU time by not preparing data for the hook that we'll then do nothing with if we don't have the hook. So using this "invoked_hook" pattern doesn't make sense in those cases. The "reference-transaction" and "prepare-commit-msg" hook also aren't racy. In those cases we'll skip the hook runs if we race with a new hook being added, whereas in the TOCTOU races being fixed here we were incorrectly skipping the required post-hook logic. 1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-03-07 13:33:46 +01:00
if (opt->invoked_hook)
*opt->invoked_hook = 1;
return 0;
}
static void run_hooks_opt_clear(struct run_hooks_opt *options)
{
strvec_clear(&options->env);
strvec_clear(&options->args);
}
int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
{
struct strbuf abs_path = STRBUF_INIT;
struct hook_cb_data cb_data = {
.rc = 0,
.hook_name = hook_name,
.options = options,
};
const char *const hook_path = find_hook(hook_name);
int jobs = 1;
int ret = 0;
if (!options)
BUG("a struct run_hooks_opt must be provided to run_hooks");
hooks: fix an obscure TOCTOU "did we just run a hook?" race Fix a Time-of-check to time-of-use (TOCTOU) race in code added in 680ee550d72 (commit: skip discarding the index if there is no pre-commit hook, 2017-08-14). This obscure race condition can occur if we e.g. ran the "pre-commit" hook and it modified the index, but hook_exists() returns false later on (e.g., because the hook itself went away, the directory became unreadable, etc.). Then we won't call discard_cache() when we should have. The race condition itself probably doesn't matter, and users would have been unlikely to run into it in practice. This problem has been noted on-list when 680ee550d72 was discussed[1], but had not been fixed. This change is mainly intended to improve the readability of the code involved, and to make reasoning about it more straightforward. It wasn't as obvious what we were trying to do here, but by having an "invoked_hook" it's clearer that e.g. our discard_cache() is happening because of the earlier hook execution. Let's also change this for the push-to-checkout hook. Now instead of checking if the hook exists and either doing a push to checkout or a push to deploy we'll always attempt a push to checkout. If the hook doesn't exist we'll fall back on push to deploy. The same behavior as before, without the TOCTOU race. See 0855331941b (receive-pack: support push-to-checkout hook, 2014-12-01) for the introduction of the previous behavior. This leaves uses of hook_exists() in two places that matter. The "reference-transaction" check in refs.c, see 67541597670 (refs: implement reference transaction hook, 2020-06-19), and the "prepare-commit-msg" hook, see 66618a50f9c (sequencer: run 'prepare-commit-msg' hook, 2018-01-24). In both of those cases we're saving ourselves CPU time by not preparing data for the hook that we'll then do nothing with if we don't have the hook. So using this "invoked_hook" pattern doesn't make sense in those cases. The "reference-transaction" and "prepare-commit-msg" hook also aren't racy. In those cases we'll skip the hook runs if we race with a new hook being added, whereas in the TOCTOU races being fixed here we were incorrectly skipping the required post-hook logic. 1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-03-07 13:33:46 +01:00
if (options->invoked_hook)
*options->invoked_hook = 0;
if (!hook_path && !options->error_if_missing)
goto cleanup;
if (!hook_path) {
ret = error("cannot find a hook named %s", hook_name);
goto cleanup;
}
cb_data.hook_path = hook_path;
if (options->dir) {
strbuf_add_absolute_path(&abs_path, hook_path);
cb_data.hook_path = abs_path.buf;
}
hook API: fix v2.36.0 regression: hooks should be connected to a TTY Fix a regression reported[1] against f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook. As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster: $ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook' #!/bin/sh seq 100000000 Benchmark 1: ./git hook run seq-hook' in 'origin/master Time (mean ± σ): 787.1 ms ± 13.6 ms [User: 701.6 ms, System: 534.4 ms] Range (min … max): 773.2 ms … 806.3 ms 10 runs Benchmark 2: ./git hook run seq-hook' in 'HEAD~0 Time (mean ± σ): 603.4 ms ± 1.6 ms [User: 573.1 ms, System: 30.3 ms] Range (min … max): 601.0 ms … 606.2 ms 10 runs Summary './git hook run seq-hook' in 'HEAD~0' ran 1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master' 1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/ Reported-by: Anthony Sottile <asottile@umich.edu> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> [jc: minor fix-up to tests for consistency] Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-06-07 10:48:20 +02:00
run_processes_parallel_ungroup = 1;
run_processes_parallel_tr2(jobs,
pick_next_hook,
notify_start_failure,
notify_hook_finished,
&cb_data,
"hook",
hook_name);
ret = cb_data.rc;
cleanup:
strbuf_release(&abs_path);
run_hooks_opt_clear(options);
return ret;
}
int run_hooks(const char *hook_name)
{
struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
return run_hooks_opt(hook_name, &opt);
}
int run_hooks_l(const char *hook_name, ...)
{
struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
va_list ap;
const char *arg;
va_start(ap, hook_name);
while ((arg = va_arg(ap, const char *)))
strvec_push(&opt.args, arg);
va_end(ap);
return run_hooks_opt(hook_name, &opt);
}