unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* segfault in T590-libconfig notmuch_database_get_config_list: closed db
@ 2020-10-27 10:09 Olivier Taïbi
  2020-10-28 11:21 ` David Bremner
  2020-10-30  0:19 ` [PATCH] lib/config: don't set destructor until iterator is initialized David Bremner
  0 siblings, 2 replies; 4+ messages in thread
From: Olivier Taïbi @ 2020-10-27 10:09 UTC (permalink / raw)
  To: notmuch

The test mentioned in the subject triggers a segfault.  This is with
xapian-core-1.4.16 on OpenBSD.  Here is the backtrace:

#0  0x000005c9d491bec2 in Xapian::TermIterator::decref() () from /usr/local/lib/libxapian.so.5.1
#1  0x000005c948827062 in Xapian::TermIterator::~TermIterator (this=0x5c9d2fdb068) at /usr/local/include/xapian/termiterator.h:85
#2  _notmuch_config_list_destroy (list=<optimized out>) at lib/config.cc:38
#3  0x000005c97e08050a in ?? () from /usr/local/lib/libtalloc.so.1.1
#4  0x000005c94882700f in notmuch_database_get_config_list (notmuch=0x5c9cb714e60, prefix=<optimized out>, out=0x7f7ffffc98e8) at lib/config.cc:137
#5  0x000005c6ff617c37 in main (argc=2, argv=0x7f7ffffc9998) at test2.c:16

For convenience, here is test2.c:
<----- begin test2.c ----->
#include <string.h>
#include <stdlib.h>
#include <notmuch-test.h>

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}

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

* Re: segfault in T590-libconfig notmuch_database_get_config_list: closed db
  2020-10-27 10:09 segfault in T590-libconfig notmuch_database_get_config_list: closed db Olivier Taïbi
@ 2020-10-28 11:21 ` David Bremner
  2020-10-30  0:19 ` [PATCH] lib/config: don't set destructor until iterator is initialized David Bremner
  1 sibling, 0 replies; 4+ messages in thread
From: David Bremner @ 2020-10-28 11:21 UTC (permalink / raw)
  To: Olivier Taïbi, notmuch

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

> The test mentioned in the subject triggers a segfault.  This is with
> xapian-core-1.4.16 on OpenBSD.  Here is the backtrace:

I started to try and look at this, but got stalled trying to build a
minimally changed upstream notmuch on openbsd. I'd prefer not to apply
22 patches before starting debugging.

d

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

* [PATCH] lib/config: don't set destructor until iterator is initialized.
  2020-10-27 10:09 segfault in T590-libconfig notmuch_database_get_config_list: closed db Olivier Taïbi
  2020-10-28 11:21 ` David Bremner
@ 2020-10-30  0:19 ` David Bremner
  2020-10-31 17:49   ` David Bremner
  1 sibling, 1 reply; 4+ messages in thread
From: David Bremner @ 2020-10-30  0:19 UTC (permalink / raw)
  To: Olivier Taïbi, notmuch; +Cc: David Bremner

As diagnosed by Olivier Taïbi in
id:20201027100916.emry3k2wujod4xnl@galois.lan, if an exception is
thrown while the initialization is happening (e.g. if the function is
called on a closed database), then the destructor is (sometimes)
invoked on an uninitialized Xapian object.

Solve the problem by moving the setting of the destructor until after
the placement new successfully completes. It is conceivable this might
cause a memory leak, but that seems preferable to crashing, and in any
case, there seems to be nothing better to be done if the
initialization is failing things are in an undefined state by
definition.
---
 lib/config.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/config.cc b/lib/config.cc
index dae0ff0e..efab01e4 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -113,7 +113,6 @@ notmuch_database_get_config_list (notmuch_database_t *notmuch,
 	goto DONE;
     }
 
-    talloc_set_destructor (list, _notmuch_config_list_destroy);
     list->notmuch = notmuch;
     list->current_key = NULL;
     list->current_val = NULL;
@@ -122,6 +121,7 @@ notmuch_database_get_config_list (notmuch_database_t *notmuch,
 
 	new(&(list->iterator)) Xapian::TermIterator (notmuch->xapian_db->metadata_keys_begin
 							 (CONFIG_PREFIX + (prefix ? prefix : "")));
+	talloc_set_destructor (list, _notmuch_config_list_destroy);
 
     } catch (const Xapian::Error &error) {
 	_notmuch_database_log (notmuch, "A Xapian exception occurred getting metadata iterator: %s.\n",
-- 
2.28.0\r

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

* Re: [PATCH] lib/config: don't set destructor until iterator is initialized.
  2020-10-30  0:19 ` [PATCH] lib/config: don't set destructor until iterator is initialized David Bremner
@ 2020-10-31 17:49   ` David Bremner
  0 siblings, 0 replies; 4+ messages in thread
From: David Bremner @ 2020-10-31 17:49 UTC (permalink / raw)
  To: Olivier Taïbi, notmuch

David Bremner <david@tethera.net> writes:

> As diagnosed by Olivier Taïbi in
> id:20201027100916.emry3k2wujod4xnl@galois.lan, if an exception is
> thrown while the initialization is happening (e.g. if the function is
> called on a closed database), then the destructor is (sometimes)
> invoked on an uninitialized Xapian object.
>
> Solve the problem by moving the setting of the destructor until after
> the placement new successfully completes. It is conceivable this might
> cause a memory leak, but that seems preferable to crashing, and in any
> case, there seems to be nothing better to be done if the
> initialization is failing things are in an undefined state by
> definition.

This one is pushed also. Let me know if it breaks something.

d

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

end of thread, other threads:[~2020-10-31 17:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 10:09 segfault in T590-libconfig notmuch_database_get_config_list: closed db Olivier Taïbi
2020-10-28 11:21 ` David Bremner
2020-10-30  0:19 ` [PATCH] lib/config: don't set destructor until iterator is initialized David Bremner
2020-10-31 17:49   ` 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).