[Lars Luthman] Bug#826843: Calls to notmuch_database_add_message() after notmuch_database_close() crash

[Lars Luthman] Bug#826843: Calls to notmuch_database_add_message() after notmuch_database_close() crash

[David Bremner]

[-- Attachment #0: Type: message/rfc822, Size: 11539 bytes --]

[-- Attachment #1.1: Type: text/plain, Size: 1959 bytes --]

Package: libnotmuch-dev
Version: 0.18.2-1
Severity: important

In the API documentation for notmuch_database_close() it says:

 * After notmuch_database_close has been called, calls to other
 * functions on objects derived from this database may either behave
 * as if the database had not been closed (e.g., if the required data
 * has been cached) or may fail with a
 * NOTMUCH_STATUS_XAPIAN_EXCEPTION.

However, if you call notmuch_database_close() on a database and then
call notmuch_database_add_message(), it doesn't fail nicely with a
NOTMUCH_STATUS_XAPIAN_EXCEPTION, it segfaults.

This should either be fixed so it doesn't crash, as documented, or the
documentation should be changed to describe which functions are unsafe
to call after notmuch_database_close().

I'm attaching a small C program to reproduce the bug. It crashes with
the following backtrace:

(gdb) bt
#0  0xb71f3f27 in Xapian::WritableDatabase::begin_transaction(bool) ()
   from /usr/lib/sse2/libxapian.so.22
#1  0xb779480d in notmuch_database_begin_atomic ()
   from /usr/lib/i386-linux-gnu/libnotmuch.so.3
#2  0xb7794fa0 in notmuch_database_add_message ()
   from /usr/lib/i386-linux-gnu/libnotmuch.so.3
#3  0x080488d3 in add_new_email (db=0x82b5610, 
    filename=0x8048b51 "/new-mail-2@example.net:2,", content=0x8048b30
"hello")
    at notmuchcrash.c:27
#4  0x080489d7 in main () at notmuchcrash.c:52


Installed version of libxapian22: 1.2.19-1+deb8u1


-- System Information:
Debian Release: 8.5
  APT prefers stable-updates
  APT policy: (500, 'stable-updates'), (500, 'stable')
Architecture: i386 (i686)

Kernel: Linux 3.16.0-4-686-pae (SMP w/2 CPU cores)
Locale: LANG=en_GB.utf8, LC_CTYPE=en_GB.utf8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)

Versions of packages libnotmuch-dev depends on:
ii  libnotmuch3  0.18.2-1

libnotmuch-dev recommends no packages.

libnotmuch-dev suggests no packages.

-- no debconf information

[-- Attachment #1.2: notmuchcrash.c --]
[-- Type: text/x-csrc, Size: 1544 bytes --]

/* Compile with:
   gcc -g -std=c99 -Wall -Werror -o notmuchcrash -lnotmuch notmuchcrash.c
*/

#define _POSIX_C_SOURCE 200809L

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include <notmuch.h>


#define DIE(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)


void add_new_email(notmuch_database_t* db,
                   char const* filename, char const* content) {
  char mail_path[64];
  strcpy(mail_path, notmuch_database_get_path(db));
  strcat(mail_path, filename);
  FILE* mf = fopen(mail_path, "w+");
  if (!mf) DIE("Failed to open mail file");
  if (fwrite(content, strlen(content), 1, mf) != 1)
    DIE("Failed to write mail");
  if (fclose(mf)) DIE("Failed to close file");
  if (notmuch_database_add_message(db, mail_path, NULL) !=
      NOTMUCH_STATUS_FILE_NOT_EMAIL)
    DIE("Something went wrong when adding the email");
}


int main() {

  /* Create new database. */
  char db_path[32];
  strcpy(db_path, "/tmp/notmuchcrash-XXXXXX");
  if (!mkdtemp(db_path)) DIE("Failed to create unique directory");
  notmuch_database_t* db;
  if (notmuch_database_create(db_path, &db)) DIE("Failed to create database");

  /* This doesn't have to be a valid email, it will crash either way. */
  char const* mail_data = "hello";

  /* First, try to add a file before closing. */
  add_new_email(db, "/new-mail-1@example.net:2,", mail_data);
  
  /* Close the database. */
  notmuch_database_close(db);

  /* Now try again. This will crash. */
  add_new_email(db, "/new-mail-2@example.net:2,", mail_data);
  
  return 0;
}

[-- Attachment #1.3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] doc: forbid further operations on a closed database

[David Bremner]

We could add many null pointer checks, but currently I don't see a use
case that justifies it.
---
 lib/notmuch.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index d4a97cb..2faa146 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -332,7 +332,9 @@ notmuch_database_status_string (const notmuch_database_t *notmuch);
  * functions on objects derived from this database may either behave
  * as if the database had not been closed (e.g., if the required data
  * has been cached) or may fail with a
- * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION. The only further operation
+ * permitted on the database itself is to call
+ * notmuch_database_destroy.
  *
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
-- 
2.8.1

Re: [PATCH] doc: forbid further operations on a closed database

[Tomi Ollila]

On Mon, Jun 27 2016, David Bremner <david@tethera.net> wrote:

> We could add many null pointer checks, but currently I don't see a use
> case that justifies it.
> ---
>  lib/notmuch.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index d4a97cb..2faa146 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -332,7 +332,9 @@ notmuch_database_status_string (const notmuch_database_t *notmuch);
>   * functions on objects derived from this database may either behave
>   * as if the database had not been closed (e.g., if the required data
>   * has been cached) or may fail with a
> - * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION. The only further operation
> + * permitted on the database itself is to call
> + * notmuch_database_destroy.

LGTM

Re: [PATCH] doc: forbid further operations on a closed database

[David Bremner]

David Bremner <david@tethera.net> writes:

> We could add many null pointer checks, but currently I don't see a use
> case that justifies it.

pushed,

d