From 8dbd1e325356ad44fbb3e2431fb4cf7c52ac0b99 Mon Sep 17 00:00:00 2001 From: Michael Vetter Date: Sun, 1 Mar 2026 00:52:13 +0100 Subject: [PATCH] fix: Fix file autocompletion bugs and restore cycling Fix several issues in cmd_ac_complete_filepath: * Prevent a segfault when input is empty. * Fix a double free where acstring was managed by both auto_gchar and GArray. * Fix a memory leak when reassigning inpcp during quote stripping. * Restore the ability to cycle through files on repeated TAB presses by caching the last input and skipping updates if the input is already a known completion. * Preserve user input style (e.g. ~, ./) in autocompletion strings to ensure matches are correctly displayed and filtered. Bug got introduced when "cleaning" code with new compiler flags and sanitizers: aec8e482687dd4d74cc009706b352b0a69e8ed5f. Add unit tests so this doesn't happen again. Before that commit we didn't use the static variable but used autocomplete_update instead. Now we avoid redundat updates and preserve the state across tab presses. We should look at this again later. Fix https://github.com/profanity-im/profanity/issues/2098 --- Makefile.am | 1 + meson.build | 1 + src/command/cmd_ac.c | 31 +++++++-- tests/unittests/command/test_cmd_ac.c | 94 +++++++++++++++++++++++++++ tests/unittests/command/test_cmd_ac.h | 9 +++ tests/unittests/unittests.c | 5 ++ 6 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 tests/unittests/command/test_cmd_ac.c create mode 100644 tests/unittests/command/test_cmd_ac.h diff --git a/Makefile.am b/Makefile.am index 411efa54..a2bc49e8 100644 --- a/Makefile.am +++ b/Makefile.am @@ -164,6 +164,7 @@ unittest_sources = \ tests/unittests/command/test_cmd_pgp.c tests/unittests/command/test_cmd_pgp.h \ tests/unittests/command/test_cmd_join.c tests/unittests/command/test_cmd_join.h \ tests/unittests/command/test_cmd_roster.c tests/unittests/command/test_cmd_roster.h \ + tests/unittests/command/test_cmd_ac.c tests/unittests/command/test_cmd_ac.h \ tests/unittests/command/test_cmd_disconnect.c tests/unittests/command/test_cmd_disconnect.h \ tests/unittests/plugins/test_callbacks.c tests/unittests/plugins/test_callbacks.h \ tests/unittests/plugins/test_plugins_disco.c tests/unittests/plugins/test_plugins_disco.h \ diff --git a/meson.build b/meson.build index 7c5f31b1..bcfe111a 100644 --- a/meson.build +++ b/meson.build @@ -628,6 +628,7 @@ if get_option('tests') 'tests/unittests/command/test_cmd_pgp.c', 'tests/unittests/command/test_cmd_join.c', 'tests/unittests/command/test_cmd_roster.c', + 'tests/unittests/command/test_cmd_ac.c', 'tests/unittests/command/test_cmd_disconnect.c', 'tests/unittests/plugins/test_callbacks.c', 'tests/unittests/plugins/test_plugins_disco.c', diff --git a/src/command/cmd_ac.c b/src/command/cmd_ac.c index 19785399..31909279 100644 --- a/src/command/cmd_ac.c +++ b/src/command/cmd_ac.c @@ -297,6 +297,8 @@ static Autocomplete vcard_set_param_ac; static Autocomplete vcard_togglable_param_ac; static Autocomplete vcard_address_type_ac; +static char* last_filepath_input = NULL; + static Autocomplete* all_acs[] = { &commands_ac, &who_room_ac, @@ -1652,6 +1654,8 @@ cmd_ac_uninit(void) autocomplete_free(plugins_unload_ac); autocomplete_free(plugins_reload_ac); autocomplete_free(script_show_ac); + g_free(last_filepath_input); + last_filepath_input = NULL; g_hash_table_destroy(ac_funcs); ac_funcs = NULL; } @@ -1684,6 +1688,7 @@ cmd_ac_complete_filepath(const char* const input, char* const startstr, gboolean *last_quote = '\0'; } gchar* unquoted = g_strdup(inpcp + 1); + g_free(inpcp); inpcp = unquoted; } @@ -1695,13 +1700,27 @@ cmd_ac_complete_filepath(const char* const input, char* const startstr, gboolean auto_gchar gchar* foofile = g_path_get_basename(expanded); auto_gchar gchar* directory = g_path_get_dirname(expanded); + // If the input is already a known completion, don't update to allow cycling + if (last_filepath_input && (g_strcmp0(inpcp, last_filepath_input) == 0 || autocomplete_contains(filepath_ac, inpcp))) { + return autocomplete_param_with_ac(input, startstr, filepath_ac, TRUE, previous); + } + // If the input ends with a slash, the basename will be "." or "/" // In that case, we are looking for all files in that directory gboolean find_all = FALSE; - if (inpcp[strlen(inpcp) - 1] == '/') { + size_t inpcp_len = strlen(inpcp); + if (inpcp_len == 0 || inpcp[inpcp_len - 1] == '/') { find_all = TRUE; } + char* last_slash = strrchr(inpcp, '/'); + auto_gchar gchar* user_dir_part = NULL; + if (last_slash) { + user_dir_part = g_strndup(inpcp, last_slash - inpcp + 1); + } else { + user_dir_part = g_strdup(""); + } + GArray* files = g_array_new(TRUE, FALSE, sizeof(gchar*)); g_array_set_clear_func(files, (GDestroyNotify)_filepath_item_free); @@ -1724,12 +1743,7 @@ cmd_ac_complete_filepath(const char* const input, char* const startstr, gboolean continue; } - auto_gchar gchar* acstring = NULL; - if (strcmp(directory, "/") == 0) { - acstring = g_strdup_printf("/%s", dir->d_name); - } else { - acstring = g_strdup_printf("%s/%s", directory, dir->d_name); - } + gchar* acstring = g_strdup_printf("%s%s", user_dir_part, dir->d_name); if (acstring) { g_array_append_val(files, acstring); @@ -1741,6 +1755,9 @@ cmd_ac_complete_filepath(const char* const input, char* const startstr, gboolean autocomplete_update(filepath_ac, (char**)files->data); g_array_free(files, TRUE); + g_free(last_filepath_input); + last_filepath_input = g_strdup(inpcp); + return autocomplete_param_with_ac(input, startstr, filepath_ac, TRUE, previous); } diff --git a/tests/unittests/command/test_cmd_ac.c b/tests/unittests/command/test_cmd_ac.c new file mode 100644 index 00000000..22ecf02c --- /dev/null +++ b/tests/unittests/command/test_cmd_ac.c @@ -0,0 +1,94 @@ +#include +#include +#include +#include +#include +#include "prof_cmocka.h" +#include "command/cmd_ac.h" + +static void +_create_test_file(const char* path) +{ + FILE* f = fopen(path, "w"); + if (f) { + fprintf(f, "test"); + fclose(f); + } +} + +void +cmd_ac_complete_filepath__segfaults_when_empty(void** state) +{ + char* result = cmd_ac_complete_filepath("/sendfile ", "/sendfile", FALSE); + + free(result); +} + +void +cmd_ac_complete_filepath__finds_files_in_current_dir(void** state) +{ + mkdir("test_dir", 0755); + _create_test_file("test_dir/file1.txt"); + _create_test_file("test_dir/file2.txt"); + + char* result = cmd_ac_complete_filepath("/sendfile test_dir/file", "/sendfile", FALSE); + assert_non_null(result); + assert_string_equal(result, "/sendfile test_dir/file1.txt"); + free(result); + + remove("test_dir/file1.txt"); + remove("test_dir/file2.txt"); + rmdir("test_dir"); +} + +void +cmd_ac_complete_filepath__finds_files_with_dot_slash(void** state) +{ + mkdir("test_dir", 0755); + _create_test_file("test_dir/file1.txt"); + + char* result = cmd_ac_complete_filepath("/sendfile ./test_dir/file", "/sendfile", FALSE); + assert_non_null(result); + assert_string_equal(result, "/sendfile ./test_dir/file1.txt"); + free(result); + + remove("test_dir/file1.txt"); + rmdir("test_dir"); +} + +void +cmd_ac_complete_filepath__cycles_through_files(void** state) +{ + mkdir("test_dir", 0755); + _create_test_file("test_dir/file1.txt"); + _create_test_file("test_dir/file2.txt"); + + // 1st TAB + char* res1 = cmd_ac_complete_filepath("/sendfile test_dir/file", "/sendfile", FALSE); + assert_non_null(res1); + assert_string_equal(res1, "/sendfile test_dir/file1.txt"); + + // 2nd TAB + char* res2 = cmd_ac_complete_filepath(res1, "/sendfile", FALSE); + assert_non_null(res2); + assert_string_equal(res2, "/sendfile test_dir/file2.txt"); + + // 3rd TAB + char* res3 = cmd_ac_complete_filepath(res2, "/sendfile", FALSE); + assert_non_null(res3); + assert_string_equal(res3, "/sendfile test_dir/file1.txt"); + + // SHIFT-TAB + char* res4 = cmd_ac_complete_filepath(res3, "/sendfile", TRUE); + assert_non_null(res4); + assert_string_equal(res4, "/sendfile test_dir/file2.txt"); + + free(res1); + free(res2); + free(res3); + free(res4); + + remove("test_dir/file1.txt"); + remove("test_dir/file2.txt"); + rmdir("test_dir"); +} diff --git a/tests/unittests/command/test_cmd_ac.h b/tests/unittests/command/test_cmd_ac.h new file mode 100644 index 00000000..e9e21b94 --- /dev/null +++ b/tests/unittests/command/test_cmd_ac.h @@ -0,0 +1,9 @@ +#ifndef TESTS_TEST_CMD_AC_H +#define TESTS_TEST_CMD_AC_H + +void cmd_ac_complete_filepath__segfaults_when_empty(void** state); +void cmd_ac_complete_filepath__finds_files_in_current_dir(void** state); +void cmd_ac_complete_filepath__finds_files_with_dot_slash(void** state); +void cmd_ac_complete_filepath__cycles_through_files(void** state); + +#endif diff --git a/tests/unittests/unittests.c b/tests/unittests/unittests.c index c0b95a19..688a7742 100644 --- a/tests/unittests/unittests.c +++ b/tests/unittests/unittests.c @@ -30,6 +30,7 @@ #include "command/test_cmd_bookmark.h" #include "command/test_cmd_join.h" #include "xmpp/test_muc.h" +#include "command/test_cmd_ac.h" #include "command/test_cmd_roster.h" #include "command/test_cmd_disconnect.h" #include "xmpp/test_form.h" @@ -96,6 +97,10 @@ main(int argc, char* argv[]) cmocka_unit_test(unique_filename_from_url__tests__table_driven), cmocka_unit_test(string_to_verbosity__returns__correct_values), cmocka_unit_test(get_expanded_path__returns__expanded), + cmocka_unit_test_setup_teardown(cmd_ac_complete_filepath__segfaults_when_empty, load_preferences, close_preferences), + cmocka_unit_test_setup_teardown(cmd_ac_complete_filepath__finds_files_in_current_dir, load_preferences, close_preferences), + cmocka_unit_test_setup_teardown(cmd_ac_complete_filepath__finds_files_with_dot_slash, load_preferences, close_preferences), + cmocka_unit_test_setup_teardown(cmd_ac_complete_filepath__cycles_through_files, load_preferences, close_preferences), cmocka_unit_test(strtoi_range__returns__true_for_valid_input), cmocka_unit_test(strtoi_range__returns__false_for_out_of_range), cmocka_unit_test(strtoi_range__returns__false_for_invalid_input),