[PATCH] zlib-related bugs
the following diff addresses 3 zlib-related bugs in notmuch. 1) the second argument of gzerror() cannot be NULL, so replace it by a dummy &errnum. 2) gzerror() cannot be closed after gzclosed(), so just print the error value instead. 3) in gz_getline(), if gz_error sets its second argument to Z_STREAM_END then there was no error (only EOF). Unfortunately the zlib manual is not very clear on the meaning of Z_STREAM_END, but I don't see how it could be an error. I found this issue by using notmuch on OpenBSD, which has an old zlib. I encountered other issues with notmuch on OpenBSD (e.g. there is no transparency mode in this older zlib, so notmuch dump output is always gzipped), but they do not seem to be bugs in notmuch. diff --git a/notmuch-dump.c b/notmuch-dump.c index 65e02639..5d4b2fa7 100644 --- a/notmuch-dump.c +++ b/notmuch-dump.c @@ -287,6 +287,7 @@ notmuch_database_dump (notmuch_database_t *notmuch, int outfd = -1; int ret = -1; +int errnum; if (output_file_name) { tempname = talloc_asprintf (notmuch, "%s.XX", output_file_name); @@ -316,7 +317,7 @@ notmuch_database_dump (notmuch_database_t *notmuch, ret = gzflush (output, Z_FINISH); if (ret) { - fprintf (stderr, "Error flushing output: %s\n", gzerror (output, NULL)); + fprintf (stderr, "Error flushing output: %s\n", gzerror (output, &errnum)); goto DONE; } @@ -332,7 +333,7 @@ notmuch_database_dump (notmuch_database_t *notmuch, ret = gzclose_w (output); if (ret) { fprintf (stderr, "Error closing %s: %s\n", name_for_error, -gzerror (output, NULL)); +gzerror (output, &errnum)); ret = EXIT_FAILURE; output = NULL; goto DONE; diff --git a/notmuch-restore.c b/notmuch-restore.c index 4b509d95..e2dc3d45 100644 --- a/notmuch-restore.c +++ b/notmuch-restore.c @@ -237,6 +237,7 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[]) int opt_index; int include = 0; int input_format = DUMP_FORMAT_AUTO; +int errnum; if (notmuch_database_open (notmuch_config_get_database_path (config), NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much)) @@ -448,10 +449,13 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[]) if (notmuch) notmuch_database_destroy (notmuch); -if (input && gzclose_r (input)) { - fprintf (stderr, "Error closing %s: %s\n", -name_for_error, gzerror (input, NULL)); - ret = EXIT_FAILURE; +if (input) { + errnum = gzclose_r (input); + if (errnum) { + fprintf (stderr, "Error closing %s: %d\n", +name_for_error, errnum); + ret = EXIT_FAILURE; + } } return ret ? EXIT_FAILURE : EXIT_SUCCESS; diff --git a/util/zlib-extra.c b/util/zlib-extra.c index f691cccf..3800fa60 100644 --- a/util/zlib-extra.c +++ b/util/zlib-extra.c @@ -47,6 +47,7 @@ gz_getline (void *talloc_ctx, char **bufptr, ssize_t *bytes_read, gzFile stream) int zlib_status = 0; (void) gzerror (stream, &zlib_status); switch (zlib_status) { + case Z_STREAM_END: case Z_OK: /* no data read before EOF */ if (offset == 0) @@ -79,8 +80,9 @@ gz_getline (void *talloc_ctx, char **bufptr, ssize_t *bytes_read, gzFile stream) const char * gz_error_string (util_status_t status, gzFile file) { +int errnum; if (status == UTIL_GZERROR) - return gzerror (file, NULL); + return gzerror (file, &errnum); else return util_error_string (status); } ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] after gzgets(), Z_STREAM_END means EOF, not error
As suggested by David Bremner in https://notmuchmail.org/pipermail/notmuch/2020/029288.html here is the patch for bug #3: after gzgets() returns NULL (meaning EOF or error), the error code Z_STREAM_END means EOF and not error. Context: I am compiling notmuch on OpenBSD which has a rather old zlib 1.2.3. It seems that the behaviour of gzgets() changed slightly between this version and more recent versions, but the manual does not reflect that change. Note that zlib's manual: - does not specify which error code (Z_OK or Z_STREAM_END) is set when EOF is reached, - does not indicate the meaning of Z_STREAM_END after gzgets(), but based on its meaning as a possible return value of inflate(), I would guess that it means EOF. PS: out of curiosity, why bother with the --gzip feature in notmuch dump and restore when the user can simply pipe to/from a gzip/bzip2/xz/... command? It seems that this feature adds complication, some of this complication being due to the fact that the behaviour of zlib is not as well-defined as stdio. Moreover using pipes allows to check that the compressed dump decompresses as it should, e.g.: $ mkfifo pipe $ dump.gz & $ notmuch dump | tee pipe | cksum $ wait && zcat dump.gz | cksum ; rm pipe This could be simplified with process substitution for shells that have this feature, and the checksum comparison can certainly be made automatic in a backup script. Disclosure: I am biased because I am currently patching notmuch-dump.c to use stdio instead of zlib in order to port notmuch to OpenBSD, since OpenBSD's older zlib does not have the "T" mode for gzopen(), so with zlib the only choice would to compress the output. Perhaps zlib will be updated in OpenBSD in the future, but this is a short-term solution that seems to not be too much trouble to maintain downstream. PPS: apart from dump and restore (and the indirect use of xapian), it seems that the only other use of zlib in notmuch is in format_part_mbox() in notmuch-show.c, which is able to read a compressed email (it seems that dovecot has an option to write emails in maildir format in this way to save space). Do I understand correctly that notmuch does not support indexing compressed email, and if so what is the point of using zlib in format_part_mbox()? diff --git a/util/zlib-extra.c b/util/zlib-extra.c index 623f6d62..2d2d2414 100644 --- a/util/zlib-extra.c +++ b/util/zlib-extra.c @@ -47,6 +47,7 @@ gz_getline (void *talloc_ctx, char **bufptr, ssize_t *bytes_read, gzFile stream) int zlib_status = 0; (void) gzerror (stream, &zlib_status); switch (zlib_status) { + case Z_STREAM_END: case Z_OK: /* no data read before EOF */ if (offset == 0) ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] gzerror() after gzclose_r() is a use after free
As suggested by David Bremner in https://notmuchmail.org/pipermail/notmuch/2020/029288.html here is a separate patch for bug #2: calling gzerror() (indirectly via gzerror_str()) after gzclose_r is a use after free, according to zlib's manual. diff --git a/notmuch-restore.c b/notmuch-restore.c index 9a8b7fb5..e2dc3d45 100644 --- a/notmuch-restore.c +++ b/notmuch-restore.c @@ -237,6 +237,7 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[]) int opt_index; int include = 0; int input_format = DUMP_FORMAT_AUTO; +int errnum; if (notmuch_database_open (notmuch_config_get_database_path (config), NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much)) @@ -448,10 +449,13 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[]) if (notmuch) notmuch_database_destroy (notmuch); -if (input && gzclose_r (input)) { - fprintf (stderr, "Error closing %s: %s\n", -name_for_error, gzerror_str (input)); - ret = EXIT_FAILURE; +if (input) { + errnum = gzclose_r (input); + if (errnum) { + fprintf (stderr, "Error closing %s: %d\n", +name_for_error, errnum); + ret = EXIT_FAILURE; + } } return ret ? EXIT_FAILURE : EXIT_SUCCESS; ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] after gzgets(), Z_STREAM_END means EOF, not error
On Tue, Apr 14, 2020 at 05:38:32PM -0300, David Bremner wrote: > Olivier Taïbi writes: > > PS: out of curiosity, why bother with the --gzip feature in notmuch dump > > and restore when the user can simply pipe to/from a gzip/bzip2/xz/... > > command? > > I believe the original motivation (in 2014) was to make the dumping > process more atomic. I guess you could dig up the mailing list > discussion from the time if you were interested. I'd be reluctant to > remove the feature after 6 six years. Thanks for the explanation. > > PPS: apart from dump and restore (and the indirect use of xapian), it > > seems that the only other use of zlib in notmuch is in > > format_part_mbox() in notmuch-show.c, which is able to read a compressed > > email (it seems that dovecot has an option to write emails in maildir > > format in this way to save space).> Do I understand correctly that > > notmuch does not support indexing compressed email, and if so what is > > the point of using zlib in format_part_mbox()? > > No, that's not correct. Notmuch does support indexing gzip compressed > mail as of version 0.29. IIRC that part uses GMime streams to do the > decompression (probably also using zlib indirectly). Thanks, that's good to know. > It would be helpful to move the discussion not intended to be part of > the commit message after --- (see https://notmuchmail.org/contributing/ > for details). Sorry about that, I read the "bugs" section of the website but did not notice these guidelines. I'll do my best to do better next time. ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] test: sort the output of the "prefix" test in T610-message-property as needed
the "prefix" test in T610-message-property extracts values from a (key,value) map where multiple entries can have the same key, and the entries are sorted by key, but not by value. The test incorrectly assumes that the values will be sorted as well, so correct this by splitting the output using head and tail and sorting each chunk using sort. NB: the relevant key/values are as follows. testkey1: alice, testvalue1, testvalue2, bob testkey3: alice3, bob3, testvalue3 --- test/T610-message-property.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh index 53a0be3b..e1be2fc3 100755 --- a/test/T610-message-property.sh +++ b/test/T610-message-property.sh @@ -186,6 +186,11 @@ EXPECT0(notmuch_message_add_property (message, "testkey3", "testvalue3")); EXPECT0(notmuch_message_add_property (message, "testkey3", "alice3")); print_properties (message, "testkey", FALSE); EOF +mv OUTPUT unsorted_OUTPUT +head -n 5 unsorted_OUTPUT | sort >OUTPUT +tail -n +6 unsorted_OUTPUT | head -n 3 | sort >>OUTPUT +tail -n +9 unsorted_OUTPUT >>OUTPUT +rm unsorted_OUTPUT cat <<'EOF' >EXPECTED == stdout == alice -- 2.26.0 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] test: sort the output of the "prefix" test in T610-message-property
This test extracts values from a (key,value) map where multiple entries can have the same key, and the entries are sorted by key, but not by value. The test incorrectly assumes that the values will be sorted as well, so sort the output. --- test/T610-message-property.sh | 12 1 file changed, 12 insertions(+) diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh index 53a0be3b..b8774230 100755 --- a/test/T610-message-property.sh +++ b/test/T610-message-property.sh @@ -186,6 +186,18 @@ EXPECT0(notmuch_message_add_property (message, "testkey3", "testvalue3")); EXPECT0(notmuch_message_add_property (message, "testkey3", "alice3")); print_properties (message, "testkey", FALSE); EOF +# expected: 4 values for testkey1, 3 values for testkey3 +# they are not guaranteed to be sorted, so sort them, leaving the first +# line '== stdout ==' and the end ('== stderr ==' and whatever error +# may have been printed) alone +mv OUTPUT unsorted_OUTPUT +awk ' NR == 1 { print; next } \ + NR < 6 { print | "sort"; next } \ + NR == 6 { close("sort") } \ + NR < 9 { print | "sort"; next } \ + NR == 9 { close("sort") } \ + { print }' unsorted_OUTPUT > OUTPUT +rm unsorted_OUTPUT cat <<'EOF' >EXPECTED == stdout == alice -- 2.26.1 --- a better version of the previous patch I sent with head/tail replaced by an awk script by Tomi Ollila which is clearer, and comments added in the test for clarity. ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
segfault in T590-libconfig notmuch_database_get_config_list: closed db
The test mentioned in the subject triggers a segfault. This is with xapian-core-1.4.16 on OpenBSD. Here is the backtrace: #0 0x05c9d491bec2 in Xapian::TermIterator::decref() () from /usr/local/lib/libxapian.so.5.1 #1 0x05c948827062 in Xapian::TermIterator::~TermIterator (this=0x5c9d2fdb068) at /usr/local/include/xapian/termiterator.h:85 #2 _notmuch_config_list_destroy (list=) at lib/config.cc:38 #3 0x05c97e08050a in ?? () from /usr/local/lib/libtalloc.so.1.1 #4 0x05c94882700f in notmuch_database_get_config_list (notmuch=0x5c9cb714e60, prefix=, out=0x7f7c98e8) at lib/config.cc:137 #5 0x05c6ff617c37 in main (argc=2, argv=0x7f7c9998) at test2.c:16 For convenience, here is test2.c: <- begin test2.c -> #include #include #include int main (int argc, char** argv) { notmuch_database_t *db; char *val; notmuch_status_t stat; EXPECT0(notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db)); { notmuch_config_list_t *list; EXPECT0(notmuch_database_close (db)); stat = notmuch_database_get_config_list (db, "nonexistent", &list); printf("%d\n", stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION); } EXPECT0(notmuch_database_destroy(db)); } <- end test2.c -> In frame 4, we see that the iterator was not initialized: (gdb) p *list $7 = {notmuch = 0x5c9cb714e60, iterator = {internal = 0xdfdfdfdfdfdfdfdf}, current_key = 0x0, current_val = 0x0} ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org