pipe_argv_from_value itself stripped the cmd-to-exec from ctx->value,
so the callers should not be doing that themselves.
Fixes #809
pipe_argv_from_value itself stripped the cmd-to-exec from ctx->value, so the callers should not be doing that themselves. Fixes #809
Thanks!
This patch handles correctly written key bindings, but there are still issues with invalid bindings. First, there's a memory leak:
==10456==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 15 byte(s) in 1 object(s) allocated from:
#0 0x7ff882c94279 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x7ff882c34363 in __interceptor_strndup /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:414
#2 0x5585d986fcfc in xstrndup ../../xmalloc.c:57
#3 0x5585d9764d9c in push_argv ../../tokenize.c:28
#4 0x5585d9765412 in tokenize_cmdline ../../tokenize.c:57
#5 0x5585d9688d5f in pipe_argv_from_value ../../config.c:1666
#6 0x5585d96899e5 in parse_key_binding_section ../../config.c:1728
#7 0x5585d968c4eb in parse_section_key_bindings ../../config.c:1889
#8 0x5585d9693bd6 in parse_config_file ../../config.c:2564
#9 0x5585d96989b5 in config_load ../../config.c:2912
#10 0x5585d96df4c5 in main ../../main.c:432
#11 0x7ff881c01b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
This can be fixed with the following patch:
diff --git a/config.c b/config.c
index 9e290c65..ef6133a3 100644
--- a/config.c
+++ b/config.c
@@ -1647,9 +1647,9 @@ argv_compare(char *const *argv1, char *const *argv2)
* - argv: allocated array containing {"cmd", "arg1", "arg2", NULL}. Caller frees.
*/
static ssize_t
-pipe_argv_from_value(struct context *ctx, char ***argv)
+pipe_argv_from_value(struct context *ctx, struct argv *argv)
{
- *argv = NULL;
+ argv->args = NULL;
if (ctx->value[0] != '[')
return 0;
@@ -1663,7 +1663,7 @@ pipe_argv_from_value(struct context *ctx, char ***argv)
size_t pipe_len = pipe_cmd_end - ctx->value - 1;
char *cmd = xstrndup(&ctx->value[1], pipe_len);
- if (!tokenize_cmdline(cmd, argv)) {
+ if (!tokenize_cmdline(cmd, &argv->args)) {
LOG_CONTEXTUAL_ERR("syntax error in command line");
free(cmd);
return -1;
@@ -1723,7 +1723,7 @@ parse_key_binding_section(struct context *ctx,
const char *const action_map[static action_count],
struct config_key_binding_list *bindings)
{
- char **pipe_argv;
+ struct argv pipe_argv;
ssize_t pipe_remove_len = pipe_argv_from_value(ctx, &pipe_argv);
if (pipe_remove_len < 0)
@@ -1738,8 +1738,8 @@ parse_key_binding_section(struct context *ctx,
/* Unset binding */
if (strcasecmp(ctx->value, "none") == 0) {
- remove_action_from_key_bindings_list(bindings, action, pipe_argv);
- free(pipe_argv);
+ remove_action_from_key_bindings_list(bindings, action, pipe_argv.args);
+ free_argv(&pipe_argv);
return true;
}
@@ -1748,12 +1748,12 @@ parse_key_binding_section(struct context *ctx,
has_key_binding_collisions(
ctx, action, action_map, bindings, &key_combos))
{
- free(pipe_argv);
+ free_argv(&pipe_argv);
free_key_combo_list(&key_combos);
return false;
}
- remove_action_from_key_bindings_list(bindings, action, pipe_argv);
+ remove_action_from_key_bindings_list(bindings, action, pipe_argv.args);
/* Emit key bindings */
size_t ofs = bindings->count;
@@ -1769,9 +1769,7 @@ parse_key_binding_section(struct context *ctx,
.modifiers = combo->modifiers,
.sym = combo->sym,
.pipe = {
- .argv = {
- .args = pipe_argv,
- },
+ .argv = pipe_argv,
.master_copy = first,
},
};
@@ -1786,7 +1784,7 @@ parse_key_binding_section(struct context *ctx,
}
LOG_CONTEXTUAL_ERR("not a valid action: %s", ctx->key);
- free(pipe_argv);
+ free_argv(&pipe_argv);
return false;
}
@@ -2099,7 +2097,7 @@ parse_section_mouse_bindings(struct context *ctx)
const char *key = ctx->key;
const char *value = ctx->value;
- char **pipe_argv;
+ struct argv pipe_argv;
ssize_t pipe_remove_len = pipe_argv_from_value(ctx, &pipe_argv);
if (pipe_remove_len < 0)
@@ -2127,7 +2125,7 @@ parse_section_mouse_bindings(struct context *ctx)
binding->action = BIND_ACTION_NONE;
}
}
- free(pipe_argv);
+ free_argv(&pipe_argv);
return true;
}
@@ -2135,7 +2133,7 @@ parse_section_mouse_bindings(struct context *ctx)
if (!value_to_mouse_combos(ctx, &key_combos) ||
has_mouse_binding_collisions(ctx, &key_combos))
{
- free(pipe_argv);
+ free_argv(&pipe_argv);
free_key_combo_list(&key_combos);
return false;
}
@@ -2145,9 +2143,9 @@ parse_section_mouse_bindings(struct context *ctx)
struct config_mouse_binding *binding = &conf->bindings.mouse.arr[i];
if (binding->action == action &&
- ((binding->pipe.argv.args == NULL && pipe_argv == NULL) ||
- (binding->pipe.argv.args != NULL && pipe_argv != NULL &&
- argv_compare(binding->pipe.argv.args, pipe_argv) == 0)))
+ ((binding->pipe.argv.args == NULL && pipe_argv.args == NULL) ||
+ (binding->pipe.argv.args != NULL && pipe_argv.args != NULL &&
+ argv_compare(binding->pipe.argv.args, pipe_argv.args) == 0)))
{
if (binding->pipe.master_copy)
free_argv(&binding->pipe.argv);
@@ -2171,9 +2169,7 @@ parse_section_mouse_bindings(struct context *ctx)
.button = combo->m.button,
.count = combo->m.count,
.pipe = {
- .argv = {
- .args = pipe_argv,
- },
+ .argv = pipe_argv,
.master_copy = first,
},
};
@@ -2187,7 +2183,7 @@ parse_section_mouse_bindings(struct context *ctx)
}
LOG_CONTEXTUAL_ERR("not a valid option: %s", key);
- free(pipe_argv);
+ free_argv(&pipe_argv);
return false;
}
But, then there's a problem with the error message. For example, this configuration (typo in Shift):
[key-bindings]
pipe-visible=[/usr/bin/false] Control+Shifts+a
Prints this:
error: /home/daniel/.config/foot/foot.ini:74: [ipe-visible].pipe-visible: Control+Shifts+a: not a valid modifier name: Shifts
Note the string inside the [] - ipe-visible. This is supposed to be the section name, i.e. key-bindings.
This is supposed to be the section name, i.e. key-bindings.
... seems to be a general issue, not specific to this patch/bug. Ignore this for now.
... seems to be a general issue, not specific to this patch/bug. Ignore this for now.
Fixed in 9b232e07f9
That leaves the argv memory leak.
Oh, I also think we should add a changelog for this, since it's a very user visible regression...
I'll just go ahead and merge this (trying to speed things up for a patch release...).
Related:
No due date set.
No dependencies set.
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?