Re: read after free in notmuch new

[David Bremner]

David Bremner  writes:

> David Bremner  writes:
>
>> I haven't had a chance to really track this down, but it seems there is
>> a memory error in notmuch new (or a maybe false positive from valgrind).
>>
>> Attached is the log from running "make memory-test OPTIONS=--medium" on
>> current git master (0e037c34).
>>
>> It looks like we talloc the message_id string with the message object as
>> parent, but it somehow outlives the message object.
>
> Sorry, that had a few commits beyond master.
>
> master (08343d3d) gives essentially the same log.
>

The log says the relevent piece of memory was freed at line 655 of database.cc, 
which
is the g_hash_table_insert in the code 

ref = _parse_message_id (ctx, refs, );

if (ref && strcmp (ref, message_id)) {
g_hash_table_insert (hash, ref, NULL);
last_ref = ref;
}


According to the docs for g_hash_table_insert

   If the key already exists in the GHashTable its current value is
   replaced with the new value. If you supplied a value_destroy_func
   when creating the GHashTable, the old value is freed using that
   function. If you supplied a key_destroy_func when creating the
   GHashTable, the passed key is freed using that function.

Since we do pass a key_destroy_func, it seems we are being naughty by
returning last_ref just below.

I'm not sure about the best solution; one option would be to drop the
key_destroy_func and manually talloc_free ref, something like

char *ref=NULL;

 while (*refs) {
if (ref) talloc_free (ref);
ref = _parse_message_id (ctx, refs, );

if (ref && strcmp (ref, message_id)) {
g_hash_table_insert (hash, ref, NULL);
last_ref = ref;
}
 }
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: notmuch version/Python bindings

[Justus Winter]

David Bremner  writes:

> Sebastian Spaeth  writes:
>
>> Hi there, I did stop using notmuch, true. Let me know if I should hand
>> over administration or if I shouldnsimply delete the package on pypi.
>>
>
> Hi Sebastian;
>
> Thanks for the followup.  If no-one steps up to maintain by the end of
> next week, I'd say just delete them from pypi.
>
> I don't really think that "the notmuch project" needs to be involved in
> deciding who, if anyone, maintains a pypi package (anymore than we
> micromanage who maintains notmuch in various linux distros). I have a
> vague memory that Justus (nominally in charge of the python bindings)
> was not interested in pypi, but I could be wrong. In any case Justus not
> very active lately either.

No, I'm really not, sorry.

Somewhat related, I adopted the Python bindings for GPGME, and I was
convinced by Python users that publishing on pypi is important for the
bindings to be used.  However, we also have the problem of people trying
to build the bindings from pypi with older versions of the library, and
one person actually suggested to build the library itself as part of
building the binginds to work around that.

Nowadays I tend to think that it is better to distribute the bindings
through the same channels as the library.


Justus




signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: add status value to _notmuch_message_ensure_metadata

[Gaute Hope]

Gaute Hope writes on februar 20, 2017 10:27:

David Bremner writes on februar 18, 2017 15:45:

In id:1487339566.mz8acpov1j.astr...@strange.none , Gaute provided a
traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
this is simple, but somewhat intrusive.

[...]

I haven't tested against Gaute's test case (needs more boost than I
have handy).


Alright then, attached is a non-boost version that takes a notmuch db
path (absolute) as the first argument (no warranty).

- gaute
# include 
# include 

# include 

using std::cout;
using std::endl;

int main (int argc, char **argv) {
  if (argc < 2) {
std::cerr << "error: specify the path to the test notmuch database" << endl;
return 1;
  }

  std::string path_db;
  path_db = argv[1];

  cout << "using db: " << path_db << endl;

  notmuch_database_t * nm_db;

  notmuch_status_t s =
notmuch_database_open (
  path_db.c_str(),
  notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_ONLY,
  _db);


  cout << "db: running test query.." << endl;
  notmuch_query_t * q = notmuch_query_create (nm_db, "*");

  unsigned int c;
  notmuch_status_t st = notmuch_query_count_threads_st (q, ); // destructive
  notmuch_query_destroy (q);
  q = notmuch_query_create (nm_db, "*");

  cout << "query: " << notmuch_query_get_query_string (q) << ", approx: "
   << c << " threads." << endl;

  notmuch_threads_t * threads;
  notmuch_thread_t  * thread;
  st = notmuch_query_search_threads_st (q, );

  std::string thread_id;

  int i = 0;

  for (; notmuch_threads_valid (threads);
 notmuch_threads_move_to_next (threads)) {
thread = notmuch_threads_get (threads);
i++;

if (i == 3)
  thread_id = notmuch_thread_get_thread_id (thread);

notmuch_thread_destroy (thread);

if (i == 3) break;
  }

  cout << "thread id to change: " << thread_id << ", thread no: " << i << endl;
  notmuch_query_destroy (q);

  /* restart query */
  cout << "restarting query.." << endl;
  q = notmuch_query_create (nm_db, "*");
  st = notmuch_query_search_threads_st (q, );

  i = 0;
  int stop = 2;

  cout << "moving to thread: " << stop << endl;
  for ( ; notmuch_threads_valid (threads);
  notmuch_threads_move_to_next (threads))
  {
thread = notmuch_threads_get (threads);
notmuch_thread_get_thread_id (thread);
i++;

cout << "tags: ";

/* get tags */
notmuch_tags_t *tags;
const char *tag;

for (tags = notmuch_thread_get_tags (thread);
 notmuch_tags_valid (tags);
 notmuch_tags_move_to_next (tags))
{
tag = notmuch_tags_get (tags);
cout << tag << " ";
}
cout << endl;

notmuch_thread_destroy (thread);

if (i == stop) break;
  }

  /* now open a new db instance, modify the already loaded thread and
   * continue loading the original query */
  notmuch_database_t * nm_db2;

  s = notmuch_database_open (
  path_db.c_str(),
  notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_WRITE,
  _db2);


  char qry_s[256];
  sprintf (qry_s, "thread:%s", thread_id.c_str ());
  notmuch_query_t * q2 = notmuch_query_create (nm_db2, qry_s);
  notmuch_threads_t * ts2;
  notmuch_thread_t  * t2;

  st = notmuch_query_search_threads_st (q2, );

  for ( ; notmuch_threads_valid (ts2);
  notmuch_threads_move_to_next (ts2))
  {
t2 = notmuch_threads_get (ts2);
std::string thread_id = notmuch_thread_get_thread_id (t2);


/* remove unread tag */
notmuch_messages_t * ms = notmuch_thread_get_messages (t2);
notmuch_message_t  * m;

for (; notmuch_messages_valid (ms); notmuch_messages_move_to_next (ms)) {
  m = notmuch_messages_get (ms);

  st = notmuch_message_remove_tag (m, "unread");

  notmuch_message_destroy (m);
}

notmuch_messages_destroy (ms);


notmuch_thread_destroy (t2);
break;
  }

  notmuch_query_destroy (q2);
  notmuch_database_close (nm_db2);

  /* re-add unread tag */
  s = notmuch_database_open (
  path_db.c_str(),
  notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_WRITE,
  _db2);



  q2 = notmuch_query_create (nm_db2, qry_s);

  st = notmuch_query_search_threads_st (q2, );

  for ( ; notmuch_threads_valid (ts2);
  notmuch_threads_move_to_next (ts2))
  {
t2 = notmuch_threads_get (ts2);
std::string thread_id = notmuch_thread_get_thread_id (t2);


/* remove unread tag */
notmuch_messages_t * ms = notmuch_thread_get_messages (t2);
notmuch_message_t  * m;

for (; notmuch_messages_valid (ms); notmuch_messages_move_to_next (ms)) {
  m = notmuch_messages_get (ms);

  st = notmuch_message_add_tag (m, "unread");

  notmuch_message_destroy (m);
}

notmuch_messages_destroy (ms);


notmuch_thread_destroy (t2);
break;
  }

  notmuch_query_destroy (q2);
  notmuch_database_close (nm_db2);

  /* continue loading */
  cout << "continue loading.." << endl;
  for ( ; notmuch_threads_valid (threads);
  notmuch_threads_move_to_next (threads))
  

Re: add status value to _notmuch_message_ensure_metadata

[Gaute Hope]

David Bremner writes on februar 18, 2017 15:45:

In id:1487339566.mz8acpov1j.astr...@strange.none , Gaute provided a
traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
this is simple, but somewhat intrusive.

[...]

I haven't tested against Gaute's test case (needs more boost than I
have handy).


Hi, thanks this seems to get rid of the hard crash, the output from my test
case is attached below.

I have not yet tried to use any new status returns, it seems that the
immediate result is that `notmuch_threads_get ()` returns a valid (but
empty?) notmuch_thread_t * which results in
`notmuch_thread_get_thread_id ()` returning NULL. Strangely, at some
seemingly arbitrary intervals the methods seem to work correctly and
return a valid `notmuch_thread_t` struct. 


`threads` and `thread` remain not NULL.


```
$ test/test_notmuch_standalone
db: running test query..
query: *, approx: 10 threads.
thread id to change: 0002, thread no: 3
restarting query..
moving to thread: 2
tags: inbox unread 
tags: inbox unread 
continue loading..

threads != NULL
thread != NULL
loading: 2: 
tags: 
threads != NULL

thread != NULL
loading: 3: 0009
tags: inbox unread 
threads != NULL

thread != NULL
loading: 4: 
tags: 
threads != NULL

thread != NULL
loading: 5: 
tags: 
threads != NULL

thread != NULL
loading: 6: 0004
tags: inbox unread 
threads != NULL

thread != NULL
loading: 7: 0006
tags: inbox unread 
threads != NULL

thread != NULL
loading: 8: 000a
tags: inbox signed unread 
threads != NULL

thread != NULL
loading: 9: 0005
tags: inbox unread 
```


with the following changes made to the test program:
```
diff --git a/test/test_notmuch_standalone.cc b/test/test_notmuch_standalone.cc
index 5ef0a00..4634986 100644
--- a/test/test_notmuch_standalone.cc
+++ b/test/test_notmuch_standalone.cc
@@ -189,8 +189,16 @@ int main () {
  cout << "threads != NULL" << endl;
}
thread = notmuch_threads_get (threads);
+if (thread == NULL) {
+  cout << "thread == NULL" << endl;
+} else {
+  cout << "thread != NULL" << endl;
+}
+
cout << "loading: " << i;
-std::string tid = notmuch_thread_get_thread_id (thread);
+const char * cid = notmuch_thread_get_thread_id (thread);
+std::string tid = "";
+if (cid != NULL) tid = cid;
cout << ": " << tid << endl;

/* get tags */
```

- gaute
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch