Fix null dereferences on unset %(fileargs) #1159
Conversation
Commit 9b262f6 (Fix null dereferences on unset state variable, 2021-07-24) fixed crashes on ":echo %(cmdlineargs)" but not on ":echo %(fileargs)". (which is embarassing because the latter is even mentioned in the commit message, but not in the test). The problem is that it fixed only format_append_argv(), which was used for %(cmdlineargs), but not for %(fileargs). For %(fileargs), argv_format() calls argv_append_array() (to append arguments verbatim). When Tig is launched without arguments, opt_file_args is NULL, so, on ":echo %(fileargs)", argv_format() does not append anything to the argv-style output array. argv_format() reports success, but leaves the output at NULL. Callers like concat_argv() however expect an argv-style array. Handle this case in argv_format(). Make sure that a successful argv_format() always return an argv-style array. This should make things simpler going forward. This allows/requires a couple of other changes: 1. src/argv.c::format_append_argv(): we can revert this change from 9b262f6 because this fix is more general. 2. src/prompt.c::run_prompt_command(): to handle "!%(cmdlineargs)" gracefully, we add a check that fails if next->argv[0] is NULL. Without this, Tig would hang (but not consume CPU). I don't know why this happens but I doubt it's worth investigating, since an empty argument array means the command is invalid anyway. This causes the change to the bang-cmdlineargs-doesnt-crash test. 3. src/prompt.c::run_prompt_command(): since argv_format() now returns valid argv-style arrays on empty input, we no longer need the check if the first argument to echo is empty. That check was also misleading: it led me to think that the subsequent call to argv_format() would *only* format argv[1], when in reality it will format argv[1], argv[2], etc., until the NULL terminator. 4. src/prompt.c::exec_run_request(): we need to adjust the check for the case where argv_format() succeeded but returns an array of length zero - now we actually get an array instead of NULL. An alternative fix would be to just declare empty results by argv_format() as failure, but that feels wrong for ":echo"
|
What about: diff --git a/src/argv.c b/src/argv.c
index 5ba7fc1..2e207a0 100644
--- a/src/argv.c
+++ b/src/argv.c
@@ -439,7 +439,7 @@ argv_format(struct argv_env *argv_env, const char ***dst_argv, const char *src_a
const char *arg = src_argv[argc];
if (!strcmp(arg, "%(fileargs)")) {
- if (file_filter && !argv_append_array(dst_argv, opt_file_args))
+ if (file_filter && !format_append_argv(&format, dst_argv, opt_file_args))
break;
} else if (!strcmp(arg, DIFF_ARGS)) {It fails the part of the test you modified but only to be consistent with the original test result (and the comment inside the test). |
| On branch master | ||
| Changes to be committed: | ||
| (no files) | ||
| Changes not staged for commit: | ||
| (no files) | ||
| Untracked files: | ||
| (no files) | ||
|
|
||
| [status] Nothing to update 100% |
There was a problem hiding this comment.
This is not consistent with the comment "This runs an empty command, hence the empty pager".
| On branch master | ||
| Changes to be committed: | ||
| (no files) | ||
| Changes not staged for commit: | ||
| (no files) | ||
| Untracked files: | ||
| (no files) | ||
|
|
||
|
|
||
|
|
||
|
|
||
| [pager] 0% | ||
| [status] Nothing to update 100% |
There was a problem hiding this comment.
Same here. In both cases, I believe the comment is correct and we should expect an empty pager.
Commit 9b262f6 (Fix null dereferences on unset state variable, 2021-07-24) fixed crashes on ":echo %(cmdlineargs)" but not on ":echo %(fileargs)". (which is embarassing because the latter is even mentioned in the commit message, but not in the test). [tk: tweaked the code and the log message to respect the original test case behaviour. Use format_append_argv() for %(fileargs) and %(revargs) as well.]
|
Sorry for the delay.
One possible surprise here is that format_append_argv() treats pathspecs as Tig format strings, for example in (We do interpret others like mainargs and cmdlineargs as format strings. I don't know if that's used anywhere.) My original fix turned Probably the current state is fine. I'm sure there is a way to simplify this string handling code next time we touch it. (BTW there's another crash on |
|
Thanks Johannes, you're right. I don't like it either, even considering the low probability. Let's revert to using argv_append() but with a check for an empty argument. It's simpler than your original proposal, still relying on diff --git a/src/argv.c b/src/argv.c
index 92403f4..b259cfc 100644
--- a/src/argv.c
+++ b/src/argv.c
@@ -441,7 +441,9 @@ argv_format(struct argv_env *argv_env, const char ***dst_argv, const char *src_a
const char *arg = src_argv[argc];
if (!strcmp(arg, "%(fileargs)")) {
- if ((flags & argv_flag_file_filter) && !format_append_argv(&format, dst_argv, opt_file_args))
+ if ((flags & argv_flag_file_filter) &&
+ ((!opt_file_args && !argv_append(dst_argv, "")) ||
+ (!argv_append_array(dst_argv, opt_file_args))))
break;
} else if (!strcmp(arg, DIFF_ARGS)) {
@@ -466,7 +468,9 @@ argv_format(struct argv_env *argv_env, const char ***dst_argv, const char *src_a
} else if (!strcmp(arg, "%(revargs)") ||
((flags & argv_flag_first) && !strcmp(arg, "%(commit)"))) {
- if ((flags & argv_flag_rev_filter) && !format_append_argv(&format, dst_argv, opt_rev_args))
+ if ((flags & argv_flag_rev_filter) &&
+ ((!opt_rev_args && !argv_append(dst_argv, "")) ||
+ (!argv_append_array(dst_argv, opt_rev_args))))
break;
} else if (!format_append_arg(&format, dst_argv, arg)) {For diff --git a/src/prompt.c b/src/prompt.c
index 13d6ae7..8a41175 100644
--- a/src/prompt.c
+++ b/src/prompt.c
@@ -798,8 +798,10 @@ prompt_toggle_option(struct view *view, const char *argv[], const char *prefix,
int i;
if (argv_size(argv) <= 2) {
- argv_free(*opt);
- (*opt)[0] = NULL;
+ if (*opt) {
+ argv_free(*opt);
+ (*opt)[0] = NULL;
+ }
return SUCCESS;
} |
|
Both sound good to me, though a ternary operator might be more intuitive here: |
No description provided.