[-- Attachment #1.1: Type: text/plain, Size: 1308 bytes --] Submitting this bug report per bremner's request (thanks for the assist today!) The documentation for 'notmuch_database_open_with_config()' states: > In case of any failure, this function returns an error status and > sets *database to NULL. However, it's possible to trigger a failure and leave *database in a partially initialized state. Usage of this pointer causes a segfault in libnotmuch. Below is a small reproducer program that causes a no config file failure and outputs if it has a non-NULL database pointer. #include <notmuch.h> #include <stdio.h> int main() { const char *db_path = "/home/aray/mail/"; notmuch_database_t *db = NULL; notmuch_status_t st = notmuch_database_open_with_config( db_path, NOTMUCH_DATABASE_MODE_READ_ONLY, NULL, NULL, &db, NULL); if (st != NOTMUCH_STATUS_SUCCESS) { printf("Received status: %s\n", notmuch_status_to_string(st)); if (db) { printf("Received non-null DB pointer\n"); return -1; } } return 0; } Please set 'db_path' to your notmuch database path and call the program like such: NOTMUCH_CONFIG="./nonexistent" ./reprod -- https://austinray.io Open Source Maintainer, Software Engineer, Keyboard Enthusiast GPG: 0127 ED83 B939 CCC9 8082 476E 1AA0 B115 C8AC 2C9E [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #1.1: Type: text/plain, Size: 1114 bytes --] Following up on this, ASAN reports a memory leak on a failure. Using 'notmuch_database_close()' or 'notmuch_database_destroy()' results in a SEGFAULT. I've updated my reproducer program to free the database on non-failure cases so it's easier to see the memory leak. #include <notmuch.h> #include <stdio.h> int main() { const char *db_path = "/home/aray/mail"; notmuch_database_t *db = NULL; notmuch_status_t st = notmuch_database_open_with_config( db_path, NOTMUCH_DATABASE_MODE_READ_ONLY, NULL, NULL, &db, NULL); if (st != NOTMUCH_STATUS_SUCCESS) { printf("Received status: %s\n", notmuch_status_to_string(st)); if (db) { printf("Received non-null DB pointer\n"); return -1; } } if (db) { notmuch_database_destroy(db); } return 0; } If there's an obvious way to fix the memory leak that I may have overlooked, please let me know. I'm not a C expert. Thanks, Austin -- https://austinray.io Open Source Maintainer, Software Engineer, Keyboard Enthusiast GPG: 0127 ED83 B939 CCC9 8082 476E 1AA0 B115 C8AC 2C9E [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --]
I'm not quite happy with the tests for notmuch_database_load_config (it's not completely trivial to trigger the specific kind of error behaviour that should result in a NULL database parameter), but here is what I have so far. In particular it fixes a bug in deallocation on the error path. I didn't run it under ASAN yet.
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 | 45 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index 59b82a6f..d2ea4a2b 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -849,4 +849,49 @@ zzzafter afterval EOF test_expect_equal_file EXPECTED OUTPUT +test_begin_subtest "open: database set to null on missing config" +test_subtest_known_broken +test_C ${MAIL_DIR} "/nonexistent" <<EOF +#include <notmuch-test.h> +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; + + notmuch_status_t st = notmuch_database_open_with_config(argv[1], + NOTMUCH_DATABASE_MODE_READ_ONLY, + argv[2], NULL, &db, NULL); + printf("db == NULL: %d\n", 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" +test_C ${MAIL_DIR} <<EOF +#include <notmuch-test.h> +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; + + notmuch_status_t st = notmuch_database_open_with_config(argv[1], + NOTMUCH_DATABASE_MODE_READ_ONLY, + NULL, NULL, &db, NULL); + printf("db == NULL: %d\n", 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 d2ea4a2b..ca70642c 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -850,7 +850,6 @@ EOF test_expect_equal_file EXPECTED OUTPUT test_begin_subtest "open: database set to null on missing config" -test_subtest_known_broken test_C ${MAIL_DIR} "/nonexistent" <<EOF #include <notmuch-test.h> int main (int argc, char **argv) { @@ -871,7 +870,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" test_C ${MAIL_DIR} <<EOF -- 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 | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/lib/notmuch.h b/lib/notmuch.h index 074fc682..571b8a87 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 + * 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 ca70642c..b36653bf 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -892,4 +892,46 @@ 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 +test_C ${MAIL_DIR} "/nonexistent" <<EOF +#include <notmuch-test.h> +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; + + notmuch_status_t st = notmuch_database_create_with_config(argv[1],argv[2], NULL, &db, NULL); + printf("db == NULL: %d\n", 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" +test_C ${MAIL_DIR} <<EOF +#include <notmuch-test.h> +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; + + notmuch_status_t st = notmuch_database_create_with_config(argv[1], + NULL, NULL, &db, NULL); + printf("db == NULL: %d\n", 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 b36653bf..cd7d6e24 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -893,7 +893,6 @@ EOF test_expect_equal_file EXPECTED OUTPUT test_begin_subtest "create: database set to null on missing config" -test_subtest_known_broken test_C ${MAIL_DIR} "/nonexistent" <<EOF #include <notmuch-test.h> int main (int argc, char **argv) { @@ -912,7 +911,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" test_C ${MAIL_DIR} <<EOF -- 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 | 2 ++ lib/open.cc | 7 +++++++ test/T590-libconfig.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/lib/notmuch.h b/lib/notmuch.h index 571b8a87..64f71280 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -458,6 +458,8 @@ notmuch_database_open_with_config (const char *database_path, * * For description of arguments, @see notmuch_database_open_with_config * + * For fatal errors, 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/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 cd7d6e24..e8fbd3d7 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -932,4 +932,44 @@ db == NULL: 1 EOF test_expect_equal_file EXPECTED OUTPUT +test_begin_subtest "load_config: database set non-null on missing config" +test_C ${MAIL_DIR} "/nonexistent" <<EOF +#include <notmuch-test.h> +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; + + notmuch_status_t st = notmuch_database_load_config(argv[1],argv[2], NULL, &db, NULL); + printf("db == NULL: %d\n", 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" +test_C ${MAIL_DIR} <<EOF +#include <notmuch-test.h> +int main (int argc, char **argv) { + notmuch_status_t stat; + notmuch_database_t *db = NULL; + + notmuch_status_t st = notmuch_database_load_config(argv[1], NULL, NULL, &db, NULL); + printf("db == NULL: %d\n", db == NULL); +} +EOF +NOTMUCH_CONFIG=${old_NOTMUCH_CONFIG} +cat <<EOF> EXPECTED +== stdout == +db == NULL: 0 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + + test_done -- 2.33.0
[-- Attachment #1.1: Type: text/plain, Size: 259 bytes --] Hi David, This resolves both the non-NULL pointer and memory leak reported by ASAN. Thanks! Austin -- https://austinray.io Open Source Maintainer, Software Engineer, Keyboard Enthusiast GPG: 0127 ED83 B939 CCC9 8082 476E 1AA0 B115 C8AC 2C9E [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --]