Notmuch 0.32 introduced some bugs in the handling of the notmuch_database_t output parameters. We didn't see them in the notmuch CLI, but they caused some crashes for neomutt. This series attempts to clean up those problems. I plan to do a point release with this series on top of 0.34.
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 59b82a6f..ed12b005 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 <<EOF > c_head3 +#include <notmuch-test.h> +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; +EOF + +cat <<EOF > 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} "/nonexistent" + notmuch_status_t st = notmuch_database_open_with_config(argv[1], + NOTMUCH_DATABASE_MODE_READ_ONLY, + argv[2], NULL, &db, NULL); +EOF +cat <<EOF> 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 <<EOF> EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + test_done -- 2.33.0
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 ed12b005..a0d70080 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -862,7 +862,6 @@ cat <<EOF > 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} "/nonexistent" 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} -- 2.33.0
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 074fc682..701eb814 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -489,6 +489,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 a0d70080..becb2821 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 <<EOF> 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 <<EOF> EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + test_done -- 2.33.0
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 becb2821..77328cd7 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} -- 2.33.0
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 701eb814..115a6a42 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -458,6 +458,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 77328cd7..e6bec872 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 <<EOF> 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 <<EOF> 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 <<EOF> EXPECTED +== stdout == +db == NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + test_done -- 2.33.0
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 e6bec872..7feb6519 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 -- 2.33.0
David Bremner <david@tethera.net> writes:
> Notmuch 0.32 introduced some bugs in the handling of the
> notmuch_database_t output parameters. We didn't see them in the
> notmuch CLI, but they caused some crashes for neomutt. This series
> attempts to clean up those problems.
>
> I plan to do a point release with this series on top of 0.34.
Series applied to release and master.
d