[PATCH] zlib-related bugs

[PATCH] zlib-related bugs

[Olivier Taïbi]

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.XXXXXX", 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, &notmuch))
@@ -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);
 }

Re: [PATCH] zlib-related bugs

[David Bremner]

Olivier Taïbi <oli@olitb.net> writes:

Thanks for the mail. In general we need each change in a separate patch
for review. Being zlib related is not really close enough for us.

> 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.

I've incorporated this change into another related series, thanks for
the report.

> 2) gzerror() cannot be closed after gzclosed(), so just print the error value
>    instead.

That seems legit. To speed things up, you could make a separate patch, rebased against
master. 

> 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.

I have to think / read about this more. A separate patch would help here
as well.

d

Re: [PATCH] zlib-related bugs

[Tomi Ollila]

On Fri, Apr 10 2020, Olivier Taïbi wrote:

> the following diff addresses 3 zlib-related bugs in notmuch.

> 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.

Interesting. What versions of gmime and xapian are you using with
notmuch on OpenBSD? IIRC(*) xapian 1.4 also wants zlib (but I am not
sure how new one).

Tomi

(*) I am building notmuch for centos 6 using this script:
  https://github.com/domo141/nottoomuch/blob/master/build/podman-notmuch-build-on-centos6.sh
  and I've configured xapian to use self-built zlib -- perhans not since
  xapian required, but that the same is used what notmuch(1) uses.