unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* v2 fix leaks in n_d_open_with_config
@ 2022-01-09 14:38 David Bremner
  2022-01-09 14:38 ` [PATCH v2 1/4] configure: check for ASAN support David Bremner
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: David Bremner @ 2022-01-09 14:38 UTC (permalink / raw)
  To: notmuch

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/4] configure: check for ASAN support
  2022-01-09 14:38 v2 fix leaks in n_d_open_with_config David Bremner
@ 2022-01-09 14:38 ` David Bremner
  2022-01-09 14:38 ` [PATCH v2 2/4] test: add known broken test for memory leaks in open David Bremner
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-01-09 14:38 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/4] test: add known broken test for memory leaks in open
  2022-01-09 14:38 v2 fix leaks in n_d_open_with_config David Bremner
  2022-01-09 14:38 ` [PATCH v2 1/4] configure: check for ASAN support David Bremner
@ 2022-01-09 14:38 ` David Bremner
  2022-01-09 14:38 ` [PATCH v2 3/4] lib/config: move g_key_File_get_string before continue David Bremner
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-01-09 14:38 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/4] lib/config: move g_key_File_get_string before continue
  2022-01-09 14:38 v2 fix leaks in n_d_open_with_config David Bremner
  2022-01-09 14:38 ` [PATCH v2 1/4] configure: check for ASAN support David Bremner
  2022-01-09 14:38 ` [PATCH v2 2/4] test: add known broken test for memory leaks in open David Bremner
@ 2022-01-09 14:38 ` David Bremner
  2022-01-09 14:38 ` [PATCH v2 4/4] lib/database: delete stemmer on destroy David Bremner
  2022-01-22 20:54 ` v2 fix leaks in n_d_open_with_config Austin Ray
  4 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-01-09 14:38 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 4/4] lib/database: delete stemmer on destroy
  2022-01-09 14:38 v2 fix leaks in n_d_open_with_config David Bremner
                   ` (2 preceding siblings ...)
  2022-01-09 14:38 ` [PATCH v2 3/4] lib/config: move g_key_File_get_string before continue David Bremner
@ 2022-01-09 14:38 ` David Bremner
  2022-01-22 20:54 ` v2 fix leaks in n_d_open_with_config Austin Ray
  4 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-01-09 14:38 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: v2 fix leaks in n_d_open_with_config
  2022-01-09 14:38 v2 fix leaks in n_d_open_with_config David Bremner
                   ` (3 preceding siblings ...)
  2022-01-09 14:38 ` [PATCH v2 4/4] lib/database: delete stemmer on destroy David Bremner
@ 2022-01-22 20:54 ` Austin Ray
  2022-01-23  1:24   ` David Bremner
  4 siblings, 1 reply; 7+ messages in thread
From: Austin Ray @ 2022-01-22 20:54 UTC (permalink / raw)
  To: David Bremner; +Cc: notmuch


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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: v2 fix leaks in n_d_open_with_config
  2022-01-22 20:54 ` v2 fix leaks in n_d_open_with_config Austin Ray
@ 2022-01-23  1:24   ` David Bremner
  0 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-01-23  1:24 UTC (permalink / raw)
  To: Austin Ray; +Cc: notmuch

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-01-23  1:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-09 14:38 v2 fix leaks in n_d_open_with_config David Bremner
2022-01-09 14:38 ` [PATCH v2 1/4] configure: check for ASAN support David Bremner
2022-01-09 14:38 ` [PATCH v2 2/4] test: add known broken test for memory leaks in open David Bremner
2022-01-09 14:38 ` [PATCH v2 3/4] lib/config: move g_key_File_get_string before continue David Bremner
2022-01-09 14:38 ` [PATCH v2 4/4] lib/database: delete stemmer on destroy David Bremner
2022-01-22 20:54 ` v2 fix leaks in n_d_open_with_config Austin Ray
2022-01-23  1:24   ` David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).