[PATCH] lib: add talloc reference from string map iterator to map

[PATCH] lib: add talloc reference from string map iterator to map

[David Bremner]

This is needed so that when the map is modified during traversal, and
thus unlinked by the database code, the map is not disposed of until the
iterator is done with it.
---

According to my obviously fallible memory, this was always intended to
work something like this. For me, this change fixes the test failures
in T610 on Debian stable.  The same bad behaviour is visible by running
T610/test6 under valgrind even in places where the test passes.

 lib/string-map.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/string-map.c b/lib/string-map.c
index 591ff6d..0bb77e9 100644
--- a/lib/string-map.c
+++ b/lib/string-map.c
@@ -170,6 +170,9 @@ _notmuch_string_map_iterator_create (notmuch_string_map_t *map, const char *key,
     if (unlikely (iter == NULL))
 	return NULL;
 
+    if (unlikely (talloc_reference (iter, map) == NULL))
+	return NULL;
+
     iter->key = talloc_strdup (iter, key);
     iter->exact = exact;
     iter->current = bsearch_first (map->pairs, map->length, key, exact);
-- 
2.1.4

Re: [PATCH] lib: add talloc reference from string map iterator to map

[Tomi Ollila]

On Fri, Sep 23 2016, David Bremner <david@tethera.net> wrote:

> This is needed so that when the map is modified during traversal, and
> thus unlinked by the database code, the map is not disposed of until the
> iterator is done with it.
> ---
>
> According to my obviously fallible memory, this was always intended to
> work something like this. For me, this change fixes the test failures
> in T610 on Debian stable.  The same bad behaviour is visible by running
> T610/test6 under valgrind even in places where the test passes.

For me the test pass on Scientific Linux 6.2, Ubuntu 16.04 and Fedora 24.

I tried to run this under valgrind ( ./T610-message-property.sh --valgrind )
but got so noisy output that I could not resolve anything definite from it.
Also /T600-named-queries.sh --val printed noisy output -- but perhaps
the noise difference (less there) can inform something.

Anyway, this seems to fix a bug that might affect someone, and at least
the situation is better with this so this should be pushed soon...

Btw: does 'Debian stable' refer to Debian 8.6(+) -- just for the record
anyone reading this message years from now :D

>  lib/string-map.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/string-map.c b/lib/string-map.c
> index 591ff6d..0bb77e9 100644
> --- a/lib/string-map.c
> +++ b/lib/string-map.c
> @@ -170,6 +170,9 @@ _notmuch_string_map_iterator_create (notmuch_string_map_t *map, const char *key,
>      if (unlikely (iter == NULL))
>  	return NULL;
>  
> +    if (unlikely (talloc_reference (iter, map) == NULL))
> +	return NULL;
> +
>      iter->key = talloc_strdup (iter, key);
>      iter->exact = exact;
>      iter->current = bsearch_first (map->pairs, map->length, key, exact);
> -- 
> 2.1.4

Re: [PATCH] lib: add talloc reference from string map iterator to map

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:
>
> I tried to run this under valgrind ( ./T610-message-property.sh --valgrind )
> but got so noisy output that I could not resolve anything definite
> from it.

Hmm. On a related topic, --valgrind seems broken here:

╭─ zancas:software/upstream/notmuch/test 
╰─ (git)-[master]-% ./T610-message-property.sh --val
./test-lib.sh:[:18: unknown condition: -lt
./test-lib.sh:27: command not found: shopt

test-lib: Testing message property API
cc1: error: unrecognised debug output level ' -O0'

All the tests fail because none of the binaries get built.

Same results if I run from bash instead of my normal zsh.

> Also /T600-named-queries.sh --val printed noisy output -- but perhaps
> the noise difference (less there) can inform something.

I ran 
% ./T610-message-property --debug
% cd tmp.T610-message-property
% LD_LIBRARY_PATH=../../lib valgrind ./test6 `pwd`/mail

>
> Anyway, this seems to fix a bug that might affect someone, and at least
> the situation is better with this so this should be pushed soon...
>
> Btw: does 'Debian stable' refer to Debian 8.6(+) -- just for the record
> anyone reading this message years from now :D

Yes, thanks for reminding me to upgrade ;).  I tested and the failure is
there with 8.6

d

Re: [PATCH] lib: add talloc reference from string map iterator to map

[Tomi Ollila]

On Sat, Sep 24 2016, David Bremner <david@tethera.net> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>
>> I tried to run this under valgrind ( ./T610-message-property.sh --valgrind )
>> but got so noisy output that I could not resolve anything definite
>> from it.
>
> Hmm. On a related topic, --valgrind seems broken here:
>
> ╭─ zancas:software/upstream/notmuch/test 
> ╰─ (git)-[master]-% ./T610-message-property.sh --val
> ./test-lib.sh:[:18: unknown condition: -lt
> ./test-lib.sh:27: command not found: shopt
>
> test-lib: Testing message property API
> cc1: error: unrecognised debug output level ' -O0'
>
> All the tests fail because none of the binaries get built.
>
> Same results if I run from bash instead of my normal zsh.
>
>> Also /T600-named-queries.sh --val printed noisy output -- but perhaps
>> the noise difference (less there) can inform something.
>
> I ran 
> % ./T610-message-property --debug
> % cd tmp.T610-message-property
> % LD_LIBRARY_PATH=../../lib valgrind ./test6 `pwd`/mail
>

Ok, I run the same before and after patching:

40  14:32  0:00  ./T610-message-property --debug
41  14:32  0:02  ./T610-message-property.sh --debug
42  14:32  0:00  cd tmp.T610-message-property/
43  14:32  0:00  ls
44  14:33  0:00  LD_LIBRARY_PATH=../../lib valgrind ./test6 $PWD/mail
45  14:33  0:25  sudo dnf install valgrind
46  14:33  0:02  LD_LIBRARY_PATH=../../lib valgrind ./test6 $PWD/mail
49  14:34  0:00  cd ..
50  14:34  0:00  cd ..
54  14:35  0:00  sc gu:tmp-m* .
57  14:35  0:00  git am tmp-mbox
58  14:35  0:07  ./configure
59  14:35  0:28  make
60  14:36  0:00  cd test/
61  14:37  0:02  ./T610-message-property.sh --debug
62  14:37  0:00  cd tmp.T610-message-property/
63  14:37  0:01  LD_LIBRARY_PATH=../../lib valgrind  ./test6 $PWD/mail
      
and can verify that valgring output was "dirty" before and clean after.

Tomi

>>
>> Anyway, this seems to fix a bug that might affect someone, and at least
>> the situation is better with this so this should be pushed soon...
>>
>> Btw: does 'Debian stable' refer to Debian 8.6(+) -- just for the record
>> anyone reading this message years from now :D
>
> Yes, thanks for reminding me to upgrade ;).  I tested and the failure is
> there with 8.6
>
> d

Re: [PATCH] lib: add talloc reference from string map iterator to map

[David Bremner]

David Bremner <david@tethera.net> writes:

> This is needed so that when the map is modified during traversal, and
> thus unlinked by the database code, the map is not disposed of until the
> iterator is done with it.
> ---

pushed,

d