Re: [PATCH v2] pager: fix crash when pager program doesn't exist
From: Jeff King <hidden>
Date: 2021-11-22 16:30:18
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2021-11-24 · Re: [PATCH v2] pager: fix crash when pager program doesn't exist · Jeff King <hidden>
- 2021-11-24 · Re: [PATCH v2] pager: fix crash when pager program doesn't exist · Ævar Arnfjörð Bjarmason <hidden>
- 2021-11-23 · Re: [PATCH v2] pager: fix crash when pager program doesn't exist · Enzo Matsumiya <hidden>
- 2021-11-22 · Re: [PATCH v2] pager: fix crash when pager program doesn't exist · Junio C Hamano <hidden>
- 2021-11-22 · Re: [PATCH v2] pager: fix crash when pager program doesn't exist · Ævar Arnfjörð Bjarmason <hidden>
On Mon, Nov 22, 2021 at 12:31:19PM -0300, Enzo Matsumiya wrote:
quoted hunk ↗ jump to hunk
On 11/21, Junio C Hamano wrote:quoted
It is rather common for us to reuse "struct child_process" in a code path, e.g. builtin/worktree.c::add_worktree() prepares a single instance of such a struct, sets cp.git_cmd to true, and runs either "update-ref" or "symbolic-ref" to update "HEAD". After successful invocation of such a git subcommand, it then runs "git reset --hard", with this piece of code:Do you agree that at least NULLing .argv and .env could be part of child_process_clear()?diff --git a/run-command.c b/run-command.c index a7bf81025afb..3839a26eff11 100644 --- a/run-command.c +++ b/run-command.c@@ -18,8 +18,9 @@ void child_process_init(struct child_process *child) void child_process_clear(struct child_process *child) { strvec_clear(&child->args); + child->argv = NULL; strvec_clear(&child->env_array); + child->env = NULL; }
It's still potentially dangerous, as callers which do not use child->args could be pointing child->argv to to a more stable array, and expecting it to be reused. It's definitely _less_ dangerous, and maybe even OK, but you'd still have to look at the callers. What would be safe is NULLing them if we know they point to the strvecs that are about to be cleared, like so:
diff --git a/run-command.c b/run-command.c
index f40df01c77..60c7331213 100644
--- a/run-command.c
+++ b/run-command.c@@ -19,6 +19,10 @@ void child_process_init(struct child_process *child) void child_process_clear(struct child_process *child) { + if (child->argv == child->args.v) + child->argv = NULL; + if (child->env == child->env_array.v) + child->env = NULL; strvec_clear(&child->args); strvec_clear(&child->env_array); }
With this change on main, all tests are successful.
That gives us some confidence, but I don't fully trust our tests to cover every corner case (the bug you found being a good example :) ). -Peff