git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Andrzej Hunt <andrzej@ahunt.org>
To: Miriam Rubio <mirucam@gmail.com>, git@vger.kernel.org
Cc: Tanushree Tumane <tanushreetumane@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C
Date: Tue, 4 May 2021 19:26:01 +0200	[thread overview]
Message-ID: <3771bfc2-e4a4-3c5d-bcf5-673b403358ca@ahunt.org> (raw)
In-Reply-To: <20210411095538.34129-4-mirucam@gmail.com>



On 11/04/2021 11:55, Miriam Rubio wrote:
> From: Tanushree Tumane <tanushreetumane@gmail.com>
> 
> Reimplement the `bisect_run()` shell function
> in C and also add `--bisect-run` subcommand to
> `git bisect--helper` to call it from git-bisect.sh.
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>   builtin/bisect--helper.c | 83 ++++++++++++++++++++++++++++++++++++++++
>   git-bisect.sh            | 62 +-----------------------------
>   2 files changed, 84 insertions(+), 61 deletions(-)
> 
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 71963979b1..dc87fc7dd0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -18,6 +18,7 @@ static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
>   static GIT_PATH_FUNC(git_path_head_name, "head-name")
>   static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
>   static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
> +static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
>   
>   static const char * const git_bisect_helper_usage[] = {
>   	N_("git bisect--helper --bisect-reset [<commit>]"),
> @@ -31,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {
>   	N_("git bisect--helper --bisect-replay <filename>"),
>   	N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
>   	N_("git bisect--helper --bisect-visualize"),
> +	N_("git bisect--helper --bisect-run <cmd>..."),
>   	NULL
>   };
>   
> @@ -1073,6 +1075,78 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
>   	return res;
>   }
>   
> +static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> +{
> +	int res = BISECT_OK;
> +	struct strbuf command = STRBUF_INIT;
> +	struct strvec args = STRVEC_INIT;
> +	struct strvec run_args = STRVEC_INIT;
> +	int exit = 0;
> +	int temporary_stdout_fd, saved_stdout;
> +
> +	if (bisect_next_check(terms, NULL))
> +		return BISECT_FAILED;
> +
> +	if (argc)
> +		sq_quote_argv(&command, argv);
> +	else
> +		return BISECT_FAILED;
> +
> +	run_args.v[0] = xstrdup(command.buf);
> +	run_args.nr = 1;

AFAIUI manipulating the strvec directly like this means that we will 
violate the promise that strvec.v is always NULL-terminated. It's 
probably safer to call 'strvec_push(run_args, command.buf)' instead of 
manipulating v and nr?

Violating the NULL-termination promise a problem because... (continued 
below)

> +
> +	while (1) {
> +		strvec_clear(&args);
> +		exit = 1;
> +
> +		printf(_("running %s"), command.buf);
> +		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

run_command_v_opt() implicitly expects a NULL-terminated list of 
strings. It's not documented in run_command_v_opt()'s comments, however 
run_command_v_opt() does explain that it's a wrapper around 
start_command(), which uses child_process, and child_process.argv is 
documented to require a NULL-terminated list.

If argv is not NULL-terminated, we hit a buffer overflow read  in 
prepare_shell_cmd(), which can be reproduced by running something like:

   make CC=clang-11 SANITIZE=address COPTS="-Og -g" GIT_TEST_OPTS=-v 
T=t6030-bisect-porcelain.sh test

which results in ASAN reporting this error:

==2116==ERROR: AddressSanitizer: global-buffer-overflow on address 
0x000001a51e28 at pc 0x0000009c6c1f bp 0x7ffcf0f60670 sp 0x7ffcf0f60668
READ of size 8 at 0x000001a51e28 thread T0
     #0 0x9c6c1e in prepare_shell_cmd run-command.c:284:8
     #1 0x9c6c1e in prepare_cmd run-command.c:419:3
     #2 0x9c6c1e in start_command run-command.c:753:6
     #3 0x9c7d35 in run_command run-command.c:1015:9
     #4 0x9c800c in run_command_v_opt_cd_env_tr2 run-command.c:1051:9
     #5 0x9c800c in run_command_v_opt_cd_env run-command.c:1033:9
     #6 0x9c800c in run_command_v_opt run-command.c:1023:9
     #7 0x4e5b60 in bisect_run builtin/bisect--helper.c:1102:9
     #8 0x4e5b60 in cmd_bisect__helper builtin/bisect--helper.c:1252:9
     #9 0x4ce8fe in run_builtin git.c:461:11
     #10 0x4ccbc8 in handle_builtin git.c:718:3
     #11 0x4cb0cc in run_argv git.c:785:4
     #12 0x4cb0cc in cmd_main git.c:916:19
     #13 0x6beded in main common-main.c:52:11
     #14 0x7f28636f7349 in __libc_start_main (/lib64/libc.so.6+0x24349)
     #15 0x420769 in _start ../sysdeps/x86_64/start.S:120

0x000001a51e28 is located 56 bytes to the left of global variable 
'config_update_recurse_submodules' defined in 'submodule.c:26:12' 
(0x1a51e60) of size 4
0x000001a51e28 is located 0 bytes to the right of global variable 
'empty_strvec' defined in 'strvec.c:5:13' (0x1a51e20) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow run-command.c:284:8 in 
prepare_shell_cmd


[... snip the rest ...]

  parent reply	other threads:[~2021-05-04 17:26 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-11  9:55 [PATCH v3 0/4] Finish converting git bisect to C part 4 Miriam Rubio
2021-04-11  9:55 ` [PATCH v3 1/4] run-command: make `exists_in_PATH()` non-static Miriam Rubio
2021-04-11  9:55 ` [PATCH v3 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C Miriam Rubio
2021-04-11 20:22   ` Junio C Hamano
2021-04-11  9:55 ` [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell " Miriam Rubio
2021-04-11 20:31   ` Junio C Hamano
2021-04-11 20:33     ` Junio C Hamano
2021-05-05  9:04     ` Christian Couder
2021-05-04 17:26   ` Andrzej Hunt [this message]
2021-05-05  2:04     ` Junio C Hamano
2021-04-11  9:55 ` [PATCH v3 4/4] bisect--helper: retire `--bisect-next-check` subcommand Miriam Rubio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3771bfc2-e4a4-3c5d-bcf5-673b403358ca@ahunt.org \
    --to=andrzej@ahunt.org \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=mirucam@gmail.com \
    --cc=tanushreetumane@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).