From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Jeff King" <peff@peff.net>, "René Scharfe" <l.s.r@web.de>,
"Git List" <git@vger.kernel.org>
Subject: Re: [PATCH v2] bisect--helper: plug strvec leak
Date: Tue, 11 Oct 2022 09:29:01 +0200 [thread overview]
Message-ID: <221011.86k056q0l4.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqq35buykz1.fsf@gitster.g>
On Mon, Oct 10 2022, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Oct 07, 2022 at 05:08:42PM +0200, René Scharfe wrote:
>>
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> index 501245fac9..28ef7ec2a4 100644
>>> --- a/builtin/bisect--helper.c
>>> +++ b/builtin/bisect--helper.c
>>> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>>> strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>>> strbuf_trim(&start_head);
>>> if (!no_checkout) {
>>> - struct strvec argv = STRVEC_INIT;
>>> + const char *argv[] = { "checkout", start_head.buf,
>>> + "--", NULL };
>>>
>>> - strvec_pushl(&argv, "checkout", start_head.buf,
>>> - "--", NULL);
>>> - if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
>>> + if (run_command_v_opt(argv, RUN_GIT_CMD)) {
>>
>> This is OK with me, but note that one thing we lose is compiler
>> protection that we remembered the trailing NULL pointer in the argv
>> array (whereas strvec_pushl() has an attribute that makes sure the last
>> argument is a NULL).
>
> The first parameter to run_command_v_opt() must be a NULL terminated
> array of strings. argv.v[] after strvec_push*() is such a NULL
> terminated array, and is suitable to be passed to the function.
>
> That much human programmers would know.
>
> But does the compiler know that run_command_v_opt() requires a NULL
> terminated array of strings, and does it know to check that argv.v[]
> came from strvec_pushl() without any annotation in the first place?
>
> For such a check to happen, I think we need to tell the compiler
> with some annotation that the first parameter to run_command_v_opt()
> is supposed to be a NULL terminated char *[] array.
>
> In the code before the patch, strvec_pushl() is annotated to require
> the last arg to be NULL, but without following data/control flow, it
> may not be clear to the compiler that argv.v[] will be NULL terminated
> after the function returns. If the compiler is sufficiently clever
> to figure it out, with the knowledge that run_command_v_opt() must
> be given a NULL terminated array of strings, we do have compiler
> protection to make the run_command_v_opt() safe.
>
> But if the code tells the compiler that run_command_v_opt() must be
> given a NULL terminated array of strings, it is obvious that the
> array passed by the code after the patch, i.e. argv[], is NULL
> terminated, and the compiler would be happy, as well.
>
>> Probably not that big a deal in practice. It would be nice if there was
>> a way to annotate this for the compiler, but I don't think there's any
>> attribute for "this pointer-to-pointer parameter should have a NULL at
>> the end".
>
> But the code before this patch is safe only for strvec_pushl() call,
> not run_command_v_opt() call, so we are not losing anything, I would
> think.
Yes, and if we suppose a bug like this sneaking in one way or the other:
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28ef7ec2a48..a7f9d43a6f1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -766,7 +766,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
strbuf_trim(&start_head);
if (!no_checkout) {
const char *argv[] = { "checkout", start_head.buf,
- "--", NULL };
+ "--" };
if (run_command_v_opt(argv, RUN_GIT_CMD)) {
res = error(_("checking out '%s' failed."
I don't know a way to statically flag that, but we'll catch it with
SANITIZE=address:
+ git bisect start 32a594a3fdac2d57cf6d02987e30eec68511498c 88bcdc1839f0ad191ffdd65cae2a2a862d682151 --
=================================================================
==1734==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe6bbf5c18 at pc 0x55d31729bc7a bp 0x7ffe6bbf5920 sp 0x7ffe6bbf5918
READ of size 8 at 0x7ffe6bbf5c18 thread T0
#0 0x55d31729bc79 in strvec_pushv strvec.c:55
#1 0x55d317232be9 in run_command_v_opt_cd_env_tr2 run-command.c:1026
#2 0x55d317232a3c in run_command_v_opt_cd_env run-command.c:1019
#3 0x55d3172329d1 in run_command_v_opt run-command.c:1009
#4 0x55d316c1337a in bisect_start builtin/bisect--helper.c:771
#5 0x55d316c17d06 in cmd_bisect__helper builtin/bisect--helper.c:1346
#6 0x55d316bf190f in run_builtin git.c:466
#7 0x55d316bf22bb in handle_builtin git.c:721
#8 0x55d316bf29e5 in run_argv git.c:788
#9 0x55d316bf375f in cmd_main git.c:921
#10 0x55d316e79a06 in main common-main.c:57
#11 0x7fceab0a0209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#12 0x7fceab0a02bb in __libc_start_main_impl ../csu/libc-start.c:389
#13 0x55d316bed210 in _start (git+0x1d7210)
So I'm not worried about it happening in the first place, but if it does
we'd also need to have no test coverage of the relevant code to miss it.
next prev parent reply other threads:[~2022-10-11 7:30 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-04 16:06 PATCH] bisect--helper: plug strvec leak in bisect_start() René Scharfe
2022-10-05 7:29 ` Ævar Arnfjörð Bjarmason
2022-10-05 15:43 ` René Scharfe
2022-10-05 19:44 ` Junio C Hamano
2022-10-06 21:35 ` Ævar Arnfjörð Bjarmason
2022-10-06 21:53 ` Junio C Hamano
2022-10-07 15:08 ` [PATCH v2] bisect--helper: plug strvec leak René Scharfe
2022-10-07 17:21 ` Junio C Hamano
2022-10-11 2:39 ` Jeff King
2022-10-11 5:42 ` Junio C Hamano
2022-10-11 7:29 ` Ævar Arnfjörð Bjarmason [this message]
2022-10-11 13:21 ` Jeff King
2022-10-11 13:20 ` Jeff King
2022-10-11 17:11 ` Junio C Hamano
2022-10-11 18:13 ` Ævar Arnfjörð Bjarmason
2022-10-11 21:43 ` Junio C Hamano
2022-10-14 19:44 ` Jeff King
2022-10-14 20:23 ` Junio C Hamano
2022-10-15 6:51 ` René Scharfe
2022-10-15 18:21 ` Jeff King
2022-10-05 19:41 ` PATCH] bisect--helper: plug strvec leak in bisect_start() Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221011.86k056q0l4.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=peff@peff.net \
/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.
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).