Re: [PATCH v2] pager: fix crash when pager program doesn't exist
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-11-22 16:28:54
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, 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; }With this change on main, all tests are successful.
That's what your patch that calls child_process_init() is trying to do, but it fails tests on "master". The "child" pointer here is valid, so what's going on with the memcpy() not working, but these two assignments doing the job? I haven't looked into why, but I'd think whatever the difference is should be moved into making child_process_init() handle that, and continuing to call that in child_process_clear(), if we go for the approach in your patch. I think an alternate direction of simply getting rid of "argv" is better in this case, and I've just submitted a topic to do that: https://lore.kernel.org/git/cover-0.5-00000000000-20211122T153605Z-avarab@gmail.com/ (local) It includes the test you noted to make the pager crash. It still leave us with this oddity: $ ~/g/git/git -c pager.show=INVALID_PAGER show HEAD error: cannot run INVALID_PAGER: No such file or directory error: cannot run INVALID_PAGER: No such file or directory But that was the case before that topic (if we hadn't crashed/segfaulted), and with your proposed change here.