I'm slightly worried that ASAN tests will prove to be flaky. If they are reliable enough, it could help catch future memory errors, which are much more dangerous than the small leak fixed here.
This will allow conditionally running tests that use the address sanitizer. --- configure | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/configure b/configure index e7bb7edc..07e0cefb 100755 --- a/configure +++ b/configure @@ -400,6 +400,18 @@ EOF exit 1 fi +printf "C compiler supports address sanitizer... " +test_cmdline="${CC} ${CFLAGS} ${CPPFLAGS} -fsanitize=address minimal.c ${LDFLAGS} -o minimal" +if ${test_cmdline} >/dev/null 2>&1 && ./minimal +then + printf "Yes.\n" + have_asan=1 +else + printf "Nope, skipping those tests.\n" + have_asan=0 +fi +unset test_cmdline + printf "Reading libnotmuch version from source... " cat > _libversion.c <<EOF #include <stdio.h> @@ -1538,6 +1550,9 @@ NOTMUCH_GMIME_X509_CERT_VALIDITY=${gmime_x509_cert_validity} # Whether GMime can verify signatures when decrypting with a session key: NOTMUCH_GMIME_VERIFY_WITH_SESSION_KEY=${gmime_verify_with_session_key} +# Does the C compiler support the address sanitizer +NOTMUCH_HAVE_ASAN=${have_asan} + # do we have man pages? NOTMUCH_HAVE_MAN=$((have_sphinx)) -- 2.34.1
This duplicates the memory leaks reported in [1] [1]: id:20220105224538.m36lnjn7rf3ieonc@athena --- test/T800-asan.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100755 test/T800-asan.sh diff --git a/test/T800-asan.sh b/test/T800-asan.sh new file mode 100755 index 00000000..8c294578 --- /dev/null +++ b/test/T800-asan.sh @@ -0,0 +1,40 @@ +#!/usr/bin/env bash +test_description='run code with ASAN enabled against the library' +. $(dirname "$0")/test-lib.sh || exit 1 + +if [ $NOTMUCH_HAVE_ASAN -ne 1 ]; then + printf "Skipping due to missing ASAN support\n" + test_done +fi + +add_email_corpus + +TEST_CFLAGS="-fsanitize=address" + +test_begin_subtest "open and destroy" +test_subtest_known_broken +test_C ${MAIL_DIR} ${NOTMUCH_CONFIG} <<EOF +#include <notmuch.h> +#include <stdio.h> + +int main(int argc, char **argv) { + 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); + if (db != NULL) + notmuch_database_destroy(db); + return 0; +} +EOF +cat <<EOF > EXPECTED +== stdout == +db != NULL: 1 +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + +test_done -- 2.34.1
In [1] Austin Ray reported some memory leaks in notmuch_database_open. One of those leaks is caused by jumping to the next key without freeing val. This change avoids that leak. [1]: id:20220105224538.m36lnjn7rf3ieonc@athena --- lib/config.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/config.cc b/lib/config.cc index 7a2882de..b9e11a54 100644 --- a/lib/config.cc +++ b/lib/config.cc @@ -435,11 +435,6 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch, for (gchar **keys_p = keys; *keys_p; keys_p++) { char *absolute_key = talloc_asprintf (notmuch, "%s.%s", *grp, *keys_p); char *normalized_val; - val = g_key_file_get_string (file, *grp, *keys_p, NULL); - if (! val) { - status = NOTMUCH_STATUS_FILE_ERROR; - goto DONE; - } /* If we opened from a given path, do not overwrite it */ if (strcmp (absolute_key, "database.path") == 0 && @@ -447,6 +442,12 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch, notmuch->xapian_db) continue; + val = g_key_file_get_string (file, *grp, *keys_p, NULL); + if (! val) { + status = NOTMUCH_STATUS_FILE_ERROR; + goto DONE; + } + normalized_val = _expand_path (notmuch, absolute_key, val); _notmuch_string_map_set (notmuch->config, absolute_key, normalized_val); g_free (val); -- 2.34.1
Commit [0] left the stemmer object accessible, but did not add de-allocation code to notmuch_database_destroy. This commit corrects that oversight. Leak originally reported by Austin Ray [1]. [0]: 3202e0d1feba1ab955ba1c07098c00208f8f0ada [1]: id:20220105224538.m36lnjn7rf3ieonc@athena --- lib/database.cc | 2 ++ test/T800-asan.sh | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/database.cc b/lib/database.cc index 6ef56d56..a012ebcf 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -753,6 +753,8 @@ notmuch_database_destroy (notmuch_database_t *notmuch) notmuch->date_range_processor = NULL; delete notmuch->last_mod_range_processor; notmuch->last_mod_range_processor = NULL; + delete notmuch->stemmer; + notmuch->stemmer = NULL; talloc_free (notmuch); diff --git a/test/T800-asan.sh b/test/T800-asan.sh index 8c294578..8607732e 100755 --- a/test/T800-asan.sh +++ b/test/T800-asan.sh @@ -12,7 +12,6 @@ add_email_corpus TEST_CFLAGS="-fsanitize=address" test_begin_subtest "open and destroy" -test_subtest_known_broken test_C ${MAIL_DIR} ${NOTMUCH_CONFIG} <<EOF #include <notmuch.h> #include <stdio.h> -- 2.34.1
[-- Attachment #1.1: Type: text/plain, Size: 440 bytes --] I can confirm this fixes the leak on my end. Thanks! > I'm slightly worried that ASAN tests will prove to be flaky. If I recall correctly, the Chromium project runs ASAN in their CI pipelines and it's pretty stable. Hopefully that's some evidence towards their stability. 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 --]
Austin Ray <austin@austinray.io> writes:
> I can confirm this fixes the leak on my end. Thanks!
>
>> I'm slightly worried that ASAN tests will prove to be flaky.
>
> If I recall correctly, the Chromium project runs ASAN in their CI
> pipelines and it's pretty stable. Hopefully that's some evidence towards
> their stability.
Series applied to master. I guess we'll just have to see about the ASAN
tests. It would be nice to have more memory checking in the test suite
if it works out.
d