From fb02817943e8cd646641d7f1fe914ce8adb3ecb2 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Thu, 21 Oct 2021 11:42:36 -0300 Subject: [PATCH 1/7] lib: document n_o_w_config can return NOTMUCH_STATUS_NO_CONFIG This should be treated as fatal by callers, since we didn't succeed in opening a Xapian database. --- lib/notmuch.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/notmuch.h b/lib/notmuch.h index 074fc682..6a0e4c51 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -433,6 +433,8 @@ notmuch_database_open_verbose (const char *path, * @retval NOTMUCH_STATUS_NULL_POINTER: The given \a database * argument is NULL. * + * @retval NOTMUCH_STATUS_NO_CONFIG: No config file was found. Fatal. + * * @retval NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory. * * @retval NOTMUCH_STATUS_FILE_ERROR: An error occurred trying to open the From a942cb8ee3f0e20d6cd72d25c432467a5ebfe93c Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 23 Oct 2021 10:22:33 -0300 Subject: [PATCH 2/7] test: add two known broken tests for missing config files The documentation claims that the database will be set to NULL in this case, but it is currently not happening. Based on a reproducer [1] from Austin Ray. [1]: id:20211021190401.imirxau2ewke6e2m@athena --- test/T590-libconfig.sh | 43 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index a9566c13..79bf5805 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -849,4 +849,47 @@ zzzafter afterval EOF test_expect_equal_file EXPECTED OUTPUT +cat < c_head3 +#include +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; +EOF + +cat < c_tail3 + printf("db == NULL: %d\n", db == NULL); +} +EOF + +test_begin_subtest "open: database set to null on missing config" +test_subtest_known_broken +cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} + notmuch_status_t st = notmuch_database_open_with_config(argv[1], + NOTMUCH_DATABASE_MODE_READ_ONLY, + "/nonexistent", NULL, &db, NULL); +EOF +cat < EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + +test_begin_subtest "open: database set to null on missing config (env)" +test_subtest_known_broken +old_NOTMUCH_CONFIG=${NOTMUCH_CONFIG} +NOTMUCH_CONFIG="/nonexistent" +cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} + notmuch_status_t st = notmuch_database_open_with_config(argv[1], + NOTMUCH_DATABASE_MODE_READ_ONLY, + NULL, NULL, &db, NULL); +EOF +NOTMUCH_CONFIG=${old_NOTMUCH_CONFIG} +cat < EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + test_done From 74c4ce6d88bcc643424c5d89cc8d30cd835e46c3 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 23 Oct 2021 10:22:34 -0300 Subject: [PATCH 3/7] lib/open: fix potential double-free, ensure *database=NULL on error During refactoring for 0.32, the code that set notmuch=NULL on various errors was moved into _finish_open. This meant that the the code which relied on that to set *database to NULL on error was no longer correct. It also introduced a potential double free, since the notmuch struct was deallocated inside _finish_open (via n_d_destroy). In this commit we revert to "allocator frees", and leave any cleanup to the caller of _finish_open. This allows us to get back the behaviour of setting *database to NULL with a small change. Other callers of _finish_open will need free notmuch on errors. --- lib/open.cc | 13 +++++-------- test/T590-libconfig.sh | 2 -- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/open.cc b/lib/open.cc index 8a835e98..77f01f72 100644 --- a/lib/open.cc +++ b/lib/open.cc @@ -396,8 +396,6 @@ _finish_open (notmuch_database_t *notmuch, " has a newer database format version (%u) than supported by this\n" " version of notmuch (%u).\n", database_path, version, NOTMUCH_DATABASE_VERSION)); - notmuch_database_destroy (notmuch); - notmuch = NULL; status = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } @@ -414,8 +412,6 @@ _finish_open (notmuch_database_t *notmuch, " requires features (%s)\n" " not supported by this version of notmuch.\n", database_path, incompat_features)); - notmuch_database_destroy (notmuch); - notmuch = NULL; status = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } @@ -489,8 +485,6 @@ _finish_open (notmuch_database_t *notmuch, } catch (const Xapian::Error &error) { IGNORE_RESULT (asprintf (&message, "A Xapian exception occurred opening database: %s\n", error.get_msg ().c_str ())); - notmuch_database_destroy (notmuch); - notmuch = NULL; status = NOTMUCH_STATUS_XAPIAN_EXCEPTION; } DONE: @@ -559,10 +553,13 @@ notmuch_database_open_with_config (const char *database_path, free (message); } + if (status && notmuch) { + notmuch_database_destroy (notmuch); + notmuch = NULL; + } + if (database) *database = notmuch; - else - talloc_free (notmuch); if (notmuch) notmuch->open = true; diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index 79bf5805..32ec072a 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -862,7 +862,6 @@ cat < c_tail3 EOF test_begin_subtest "open: database set to null on missing config" -test_subtest_known_broken cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} notmuch_status_t st = notmuch_database_open_with_config(argv[1], NOTMUCH_DATABASE_MODE_READ_ONLY, @@ -876,7 +875,6 @@ EOF test_expect_equal_file EXPECTED OUTPUT test_begin_subtest "open: database set to null on missing config (env)" -test_subtest_known_broken old_NOTMUCH_CONFIG=${NOTMUCH_CONFIG} NOTMUCH_CONFIG="/nonexistent" cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} From f3fcdd2ddac2384c0c1daeffa495ad3ec01bc1af Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 23 Oct 2021 10:22:35 -0300 Subject: [PATCH 4/7] lib/create: document expectations for db on error, add tests It seems sensible to harmonize the behaviour with n_d_open_with_config. In this commit we just assert the desired behaviour. --- lib/notmuch.h | 3 +++ test/T590-libconfig.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/lib/notmuch.h b/lib/notmuch.h index 6a0e4c51..316385b8 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -491,6 +491,9 @@ notmuch_database_load_config (const char *database_path, * * For description of arguments, @see notmuch_database_open_with_config * + * In case of any failure, this function returns an error status and + * sets *database to NULL. + * * @retval NOTMUCH_STATUS_SUCCESS: Successfully created the database. * * @retval NOTMUCH_STATUS_DATABASE_EXISTS: Database already exists, not created diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index 32ec072a..adb9b6e7 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -890,4 +890,32 @@ db == NULL: 1 EOF test_expect_equal_file EXPECTED OUTPUT +test_begin_subtest "create: database set to null on missing config" +test_subtest_known_broken +cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} "/nonexistent" + notmuch_status_t st = notmuch_database_create_with_config(argv[1],argv[2], NULL, &db, NULL); +EOF +cat < EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + +test_begin_subtest "create: database set to null on missing config (env)" +test_subtest_known_broken +old_NOTMUCH_CONFIG=${NOTMUCH_CONFIG} +NOTMUCH_CONFIG="/nonexistent" +cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} + notmuch_status_t st = notmuch_database_create_with_config(argv[1], + NULL, NULL, &db, NULL); +EOF +NOTMUCH_CONFIG=${old_NOTMUCH_CONFIG} +cat < EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + test_done From 2ba50b52302dce08068843e0029f9ee935a0d7f3 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 23 Oct 2021 10:22:36 -0300 Subject: [PATCH 5/7] lib/create: fix memory leak, ensure *database=NULL on error This code previously relied on _finish_open to free the notmuch struct on errors (except for the case of database == NULL, which was a potential double free). When we removed those frees from _finish_open, we introduced a (small) memory leak. In this commit, fix the memory leak, and harmonize the on-error behaviour with n_d_open_with_config. --- lib/open.cc | 10 ++++++++-- test/T590-libconfig.sh | 2 -- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/open.cc b/lib/open.cc index 77f01f72..6fa00a84 100644 --- a/lib/open.cc +++ b/lib/open.cc @@ -714,10 +714,16 @@ notmuch_database_create_with_config (const char *database_path, else free (message); } + if (status && notmuch) { + notmuch_database_destroy (notmuch); + notmuch = NULL; + } + if (database) *database = notmuch; - else - talloc_free (notmuch); + + if (notmuch) + notmuch->open = true; return status; } diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index adb9b6e7..bed887aa 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -891,7 +891,6 @@ EOF test_expect_equal_file EXPECTED OUTPUT test_begin_subtest "create: database set to null on missing config" -test_subtest_known_broken cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} "/nonexistent" notmuch_status_t st = notmuch_database_create_with_config(argv[1],argv[2], NULL, &db, NULL); EOF @@ -903,7 +902,6 @@ EOF test_expect_equal_file EXPECTED OUTPUT test_begin_subtest "create: database set to null on missing config (env)" -test_subtest_known_broken old_NOTMUCH_CONFIG=${NOTMUCH_CONFIG} NOTMUCH_CONFIG="/nonexistent" cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} From 8f0b84789d4fda0182e073b68b73160c9b94a4b7 Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 23 Oct 2021 10:22:37 -0300 Subject: [PATCH 6/7] lib/load_config: document expectations for db on error, add tests This is a bit different than n_d_{open,create}_with_config, since there are several non-zero status codes where we do want to return a non-NULL database structure. --- lib/notmuch.h | 3 +++ test/T590-libconfig.sh | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/lib/notmuch.h b/lib/notmuch.h index 316385b8..5c5a024e 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -460,6 +460,9 @@ notmuch_database_open_with_config (const char *database_path, * * For description of arguments, @see notmuch_database_open_with_config * + * For errors other then NO_DATABASE and NO_CONFIG, *database is set to + * NULL. + * * @retval NOTMUCH_STATUS_SUCCESS: Successfully loaded configuration. * * @retval NOTMUCH_STATUS_NO_CONFIG: No config file was loaded. Not fatal. diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index bed887aa..b4abbd78 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -916,4 +916,41 @@ db == NULL: 1 EOF test_expect_equal_file EXPECTED OUTPUT +test_begin_subtest "load_config: database set non-null on missing config" +cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} "/nonexistent" + notmuch_status_t st = notmuch_database_load_config(argv[1],argv[2], NULL, &db, NULL); +EOF +cat < EXPECTED +== stdout == +db == NULL: 0 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + +test_begin_subtest "load_config: database non-null on missing config (env)" +old_NOTMUCH_CONFIG=${NOTMUCH_CONFIG} +NOTMUCH_CONFIG="/nonexistent" +cat c_head3 - c_tail3 <<'EOF' | test_C ${MAIL_DIR} + notmuch_status_t st = notmuch_database_load_config(argv[1], NULL, NULL, &db, NULL); +EOF +NOTMUCH_CONFIG=${old_NOTMUCH_CONFIG} +cat < EXPECTED +== stdout == +db == NULL: 0 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + +test_begin_subtest "load_config: database set to NULL on fatal error" +test_subtest_known_broken +cat c_head3 - c_tail3 <<'EOF' | test_C + notmuch_status_t st = notmuch_database_load_config("relative", NULL, NULL, &db, NULL); +EOF +cat < EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + test_done From fe51c275fcd7107d92b40de511086300ba4060ed Mon Sep 17 00:00:00 2001 From: David Bremner Date: Sat, 23 Oct 2021 10:22:38 -0300 Subject: [PATCH 7/7] lib/load_config: deallocate / NULL database on fatal error This fixes a potential memory leak, and makes the behaviour of notmuch_database_load_config (somewhat) consistent with n_d_{open,create} with config. --- lib/open.cc | 7 +++++++ test/T590-libconfig.sh | 1 - 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/open.cc b/lib/open.cc index 6fa00a84..ba32c2f1 100644 --- a/lib/open.cc +++ b/lib/open.cc @@ -871,6 +871,13 @@ notmuch_database_load_config (const char *database_path, if (status_string) *status_string = message; + if (status && + status != NOTMUCH_STATUS_NO_DATABASE + && status != NOTMUCH_STATUS_NO_CONFIG) { + notmuch_database_destroy (notmuch); + notmuch = NULL; + } + if (database) *database = notmuch; diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index b4abbd78..8db51ed0 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -942,7 +942,6 @@ EOF test_expect_equal_file EXPECTED OUTPUT test_begin_subtest "load_config: database set to NULL on fatal error" -test_subtest_known_broken cat c_head3 - c_tail3 <<'EOF' | test_C notmuch_status_t st = notmuch_database_load_config("relative", NULL, NULL, &db, NULL); EOF