segfault if notmuch-show query has spurious .. (w/ v0.17)

segfault if notmuch-show query has spurious .. (w/ v0.17)

[Sanjoy Mahajan]

Probably because I kept using notmuch-emacs .elc code from 0.16 after
notmuch got upgraded to 0.17 (I rarely restart emacs), my Emacs
interface to notmuch started generating queries that caused Xapian
exceptions and segfaults.  Here's one:

  $ notmuch show '( FW: Student Employment Orie.. )'
  A Xapian exception occurred performing query: Unknown range operation
  Query string was: ( FW: Student Employment Orie.. )
  Segmentation fault

Restarting Emacs stopped those queries, so I don't think that's an
issue.  However, notmuch itself probably should not segfault, even if
Xapian gets confused by the .. in the query (making it look like a date
range).

This is on Debian/i386 with the notmuch 0.17-3 packages.
-- 
-Sanjoy

<http://savelongwharfpark.org/>
Save Long Wharf Park in Boston Harbor!

<http://streetfightingmath.com/>
Six reasoning tools to make hard problems easy.

Re: segfault if notmuch-show query has spurious .. (w/ v0.17)

[David Bremner]

Sanjoy Mahajan <sanjoy@olin.edu> writes:

> Probably because I kept using notmuch-emacs .elc code from 0.16 after
> notmuch got upgraded to 0.17 (I rarely restart emacs), my Emacs
> interface to notmuch started generating queries that caused Xapian
> exceptions and segfaults.  Here's one:
>
>   $ notmuch show '( FW: Student Employment Orie.. )'
>   A Xapian exception occurred performing query: Unknown range operation
>   Query string was: ( FW: Student Employment Orie.. )
>   Segmentation fault
>
> Restarting Emacs stopped those queries, so I don't think that's an
> issue.  However, notmuch itself probably should not segfault, even if
> Xapian gets confused by the .. in the query (making it look like a date
> range).
>

Looking at the example code in lib/notmuch.h (which, surprise, we use in
notmuch-show), we see

     for (threads = notmuch_query_search_threads (query);
          notmuch_threads_valid (threads);
          notmuch_threads_move_to_next (threads))
     {
         thread = notmuch_threads_get (threads);
         ....
         notmuch_thread_destroy (thread);
     }

notmuch_query_search_theads documents that it might return NULL, but
notmuch_threads_valid does not handle NULL input. It seems to me that
notmuch_threads_valid should just return FALSE on NULL input.  

d

[PATCH] lib: make notmuch_threads_valid return FALSE when passed NULL

[David Bremner]

Without this patch, the example code in the header docs crashes for certain
invalid queries (see id:871u00oimv.fsf@approx.mit.edu)
---
 lib/notmuch.h | 2 ++
 lib/query.cc  | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 02604c5..68896ae 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -802,6 +802,8 @@ notmuch_query_destroy (notmuch_query_t *query);
  * valid object. Whereas when this function returns FALSE,
  * notmuch_threads_get will return NULL.
  *
+ * If passed a NULL pointer, this function returns FALSE
+ *
  * See the documentation of notmuch_query_search_threads for example
  * code showing how to iterate over a notmuch_threads_t object.
  */
diff --git a/lib/query.cc b/lib/query.cc
index ec60e2e..60ff8bd 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -462,6 +462,9 @@ notmuch_threads_valid (notmuch_threads_t *threads)
 {
     unsigned int doc_id;
 
+    if (! threads)
+	return FALSE;
+
     while (threads->doc_id_pos < threads->doc_ids->len) {
 	doc_id = g_array_index (threads->doc_ids, unsigned int,
 				threads->doc_id_pos);
-- 
1.8.5.2

Re: [PATCH] lib: make notmuch_threads_valid return FALSE when passed NULL

[Tomi Ollila]

On Wed, Jan 22 2014, David Bremner <david@tethera.net> wrote:

> Without this patch, the example code in the header docs crashes for certain
> invalid queries (see id:871u00oimv.fsf@approx.mit.edu)
> ---

Looks good and seems to work:

$ make
...
$ notmuch show foo..
A Xapian exception occurred performing query: Unknown range operation
Query string was: foo..
zsh: segmentation fault  notmuch show foo..
$ ./notmuch show foo..
A Xapian exception occurred performing query: Unknown range operation
Query string was: foo..

(i.e. no "segmentation fault" in the latter)

Tomi


>  lib/notmuch.h | 2 ++
>  lib/query.cc  | 3 +++
>  2 files changed, 5 insertions(+)
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 02604c5..68896ae 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -802,6 +802,8 @@ notmuch_query_destroy (notmuch_query_t *query);
>   * valid object. Whereas when this function returns FALSE,
>   * notmuch_threads_get will return NULL.
>   *
> + * If passed a NULL pointer, this function returns FALSE
> + *
>   * See the documentation of notmuch_query_search_threads for example
>   * code showing how to iterate over a notmuch_threads_t object.
>   */
> diff --git a/lib/query.cc b/lib/query.cc
> index ec60e2e..60ff8bd 100644
> --- a/lib/query.cc
> +++ b/lib/query.cc
> @@ -462,6 +462,9 @@ notmuch_threads_valid (notmuch_threads_t *threads)
>  {
>      unsigned int doc_id;
>  
> +    if (! threads)

This format seems to be consistent with surrounding code (vs. threads == NULL)

> +	return FALSE;
> +
>      while (threads->doc_id_pos < threads->doc_ids->len) {
>  	doc_id = g_array_index (threads->doc_ids, unsigned int,
>  				threads->doc_id_pos);
> -- 
> 1.8.5.2

Round 2: fix for notmuch show segfault

[David Bremner]

Second try. The second patch is now not really needed (although I
didn't test without it). If omitted, the library docs should probably
be updated.

[PATCH 1/3] test: add known broken test exit code of notmuch show

[David Bremner]

This test catches a segfault on a syntactically invalid query. It also
catches a problem with my initial fix, which still returned 0.
---
 test/T520-show.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100755 test/T520-show.sh

diff --git a/test/T520-show.sh b/test/T520-show.sh
new file mode 100755
index 0000000..bdd9d71
--- /dev/null
+++ b/test/T520-show.sh
@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+test_description='"notmuch show"'
+
+. ./test-lib.sh
+
+add_email_corpus
+
+test_begin_subtest "exit code for show invalid query"
+test_subtest_known_broken
+notmuch show foo..
+exit_code=$?
+test_expect_equal 1 $exit_code
+
+test_done
-- 
1.8.5.2

[PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL

[David Bremner]

Without this patch, the example code in the header docs crashes for certain
invalid queries (see id:871u00oimv.fsf@approx.mit.edu)
---
 lib/notmuch.h | 2 ++
 lib/query.cc  | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 02604c5..68896ae 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -802,6 +802,8 @@ notmuch_query_destroy (notmuch_query_t *query);
  * valid object. Whereas when this function returns FALSE,
  * notmuch_threads_get will return NULL.
  *
+ * If passed a NULL pointer, this function returns FALSE
+ *
  * See the documentation of notmuch_query_search_threads for example
  * code showing how to iterate over a notmuch_threads_t object.
  */
diff --git a/lib/query.cc b/lib/query.cc
index ec60e2e..60ff8bd 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -462,6 +462,9 @@ notmuch_threads_valid (notmuch_threads_t *threads)
 {
     unsigned int doc_id;
 
+    if (! threads)
+	return FALSE;
+
     while (threads->doc_id_pos < threads->doc_ids->len) {
 	doc_id = g_array_index (threads->doc_ids, unsigned int,
 				threads->doc_id_pos);
-- 
1.8.5.2

[PATCH 3/3] notmuch-show: detect xapian exception in query

[David Bremner]

We want to return an error status, not 0 or (worse) segfault.
---
 notmuch-show.c    | 6 +++++-
 test/T520-show.sh | 1 -
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 528694b..b162738 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1015,9 +1015,13 @@ do_show (void *ctx,
     notmuch_messages_t *messages;
     notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
 
+    threads = notmuch_query_search_threads (query);
+    if (! threads)
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+
     sp->begin_list (sp);
 
-    for (threads = notmuch_query_search_threads (query);
+    for ( ;
 	 notmuch_threads_valid (threads);
 	 notmuch_threads_move_to_next (threads))
     {
diff --git a/test/T520-show.sh b/test/T520-show.sh
index bdd9d71..0657c99 100755
--- a/test/T520-show.sh
+++ b/test/T520-show.sh
@@ -6,7 +6,6 @@ test_description='"notmuch show"'
 add_email_corpus
 
 test_begin_subtest "exit code for show invalid query"
-test_subtest_known_broken
 notmuch show foo..
 exit_code=$?
 test_expect_equal 1 $exit_code
-- 
1.8.5.2

Re: Round 2: fix for notmuch show segfault

[Mark Walters]

This looks good and works (tested without patch 2/3). I don't have a
view on whether we should do the second patch or just update the docs.

Best wishes

Mark



On Thu, 23 Jan 2014, David Bremner <david@tethera.net> wrote:
> Second try. The second patch is now not really needed (although I
> didn't test without it). If omitted, the library docs should probably
> be updated.
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: Round 2: fix for notmuch show segfault

[Tomi Ollila]

On Thu, Jan 23 2014, Mark Walters <markwalters1009@gmail.com> wrote:

> This looks good and works (tested without patch 2/3). I don't have a
> view on whether we should do the second patch or just update the docs.

Looks good. for patch #2 I'd check which one is more consistent
with other code constructs used & library interface functions...

Tomi


>
> Best wishes
>
> Mark
>
>
>
> On Thu, 23 Jan 2014, David Bremner <david@tethera.net> wrote:
>> Second try. The second patch is now not really needed (although I
>> didn't test without it). If omitted, the library docs should probably
>> be updated.
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL

[Jani Nikula]

On Thu, 23 Jan 2014, David Bremner <david@tethera.net> wrote:
> Without this patch, the example code in the header docs crashes for certain
> invalid queries (see id:871u00oimv.fsf@approx.mit.edu)
> ---
>  lib/notmuch.h | 2 ++
>  lib/query.cc  | 3 +++
>  2 files changed, 5 insertions(+)
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 02604c5..68896ae 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -802,6 +802,8 @@ notmuch_query_destroy (notmuch_query_t *query);
>   * valid object. Whereas when this function returns FALSE,
>   * notmuch_threads_get will return NULL.
>   *
> + * If passed a NULL pointer, this function returns FALSE
> + *
>   * See the documentation of notmuch_query_search_threads for example
>   * code showing how to iterate over a notmuch_threads_t object.
>   */
> diff --git a/lib/query.cc b/lib/query.cc
> index ec60e2e..60ff8bd 100644
> --- a/lib/query.cc
> +++ b/lib/query.cc
> @@ -462,6 +462,9 @@ notmuch_threads_valid (notmuch_threads_t *threads)
>  {
>      unsigned int doc_id;
>  
> +    if (! threads)
> +	return FALSE;
> +

LGTM, and this is in line with notmuch_messages_valid().

BR,
Jani.

>      while (threads->doc_id_pos < threads->doc_ids->len) {
>  	doc_id = g_array_index (threads->doc_ids, unsigned int,
>  				threads->doc_id_pos);
> -- 
> 1.8.5.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/3] notmuch-show: detect xapian exception in query

[Jani Nikula]

On Thu, 23 Jan 2014, David Bremner <david@tethera.net> wrote:
> We want to return an error status, not 0 or (worse) segfault.
> ---
>  notmuch-show.c    | 6 +++++-
>  test/T520-show.sh | 1 -
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 528694b..b162738 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -1015,9 +1015,13 @@ do_show (void *ctx,
>      notmuch_messages_t *messages;
>      notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
>  
> +    threads = notmuch_query_search_threads (query);
> +    if (! threads)
> +	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;

This should just return 1 or something. See how the function eventually
returns res != NOTMUCH_STATUS_SUCCESS instead of notmuch_status_t. And
threads == NULL is not guaranteed to mean an exception occurred anyway.

Otherwise the patch LGTM, and is in line with the error handling in
notmuch search, which does not segfault on similar queries.

BR,
Jani.


> +
>      sp->begin_list (sp);
>  
> -    for (threads = notmuch_query_search_threads (query);
> +    for ( ;
>  	 notmuch_threads_valid (threads);
>  	 notmuch_threads_move_to_next (threads))
>      {
> diff --git a/test/T520-show.sh b/test/T520-show.sh
> index bdd9d71..0657c99 100755
> --- a/test/T520-show.sh
> +++ b/test/T520-show.sh
> @@ -6,7 +6,6 @@ test_description='"notmuch show"'
>  add_email_corpus
>  
>  test_begin_subtest "exit code for show invalid query"
> -test_subtest_known_broken
>  notmuch show foo..
>  exit_code=$?
>  test_expect_equal 1 $exit_code
> -- 
> 1.8.5.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/3] notmuch-show: detect xapian exception in query

[David Bremner]

Jani Nikula <jani@nikula.org> writes:
>
> This should just return 1 or something. See how the function eventually
> returns res != NOTMUCH_STATUS_SUCCESS instead of notmuch_status_t. And
> threads == NULL is not guaranteed to mean an exception occurred anyway.
>
> Otherwise the patch LGTM, and is in line with the error handling in
> notmuch search, which does not segfault on similar queries.

pushed a version amended as suggested.

d

Re: segfault if notmuch-show query has spurious .. (w/ v0.17)

[David Bremner]

Sanjoy Mahajan <sanjoy@olin.edu> writes:

> Probably because I kept using notmuch-emacs .elc code from 0.16 after
> notmuch got upgraded to 0.17 (I rarely restart emacs), my Emacs
> interface to notmuch started generating queries that caused Xapian
> exceptions and segfaults.  Here's one:
>
>   $ notmuch show '( FW: Student Employment Orie.. )'
>   A Xapian exception occurred performing query: Unknown range operation
>   Query string was: ( FW: Student Employment Orie.. )
>   Segmentation fault

this should be fixed in git master (and in the next release of notmuch).

d