v2 fix leaks in n_d_open_with_config

v2 fix leaks in n_d_open_with_config

[David Bremner]

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.

[PATCH v2 1/4] configure: check for ASAN support

[David Bremner]

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

[PATCH v2 2/4] test: add known broken test for memory leaks in open

[David Bremner]

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

[PATCH v2 3/4] lib/config: move g_key_File_get_string before continue

[David Bremner]

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

[PATCH v2 4/4] lib/database: delete stemmer on destroy

[David Bremner]

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

Re: v2 fix leaks in n_d_open_with_config

[Austin Ray]

[-- 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 --]

Re: v2 fix leaks in n_d_open_with_config

[David Bremner]

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