* segfault if notmuch-show query has spurious .. (w/ v0.17)
@ 2014-01-22 2:27 Sanjoy Mahajan
2014-01-22 12:40 ` David Bremner
2014-01-25 1:01 ` segfault if notmuch-show query has spurious .. (w/ v0.17) David Bremner
0 siblings, 2 replies; 14+ messages in thread
From: Sanjoy Mahajan @ 2014-01-22 2:27 UTC (permalink / raw)
To: notmuch
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: segfault if notmuch-show query has spurious .. (w/ v0.17)
2014-01-22 2:27 segfault if notmuch-show query has spurious .. (w/ v0.17) Sanjoy Mahajan
@ 2014-01-22 12:40 ` David Bremner
2014-01-22 16:42 ` [PATCH] lib: make notmuch_threads_valid return FALSE when passed NULL David Bremner
2014-01-25 1:01 ` segfault if notmuch-show query has spurious .. (w/ v0.17) David Bremner
1 sibling, 1 reply; 14+ messages in thread
From: David Bremner @ 2014-01-22 12:40 UTC (permalink / raw)
To: Sanjoy Mahajan, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] lib: make notmuch_threads_valid return FALSE when passed NULL
2014-01-22 12:40 ` David Bremner
@ 2014-01-22 16:42 ` David Bremner
2014-01-22 17:30 ` Tomi Ollila
0 siblings, 1 reply; 14+ messages in thread
From: David Bremner @ 2014-01-22 16:42 UTC (permalink / raw)
To: notmuch
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] lib: make notmuch_threads_valid return FALSE when passed NULL
2014-01-22 16:42 ` [PATCH] lib: make notmuch_threads_valid return FALSE when passed NULL David Bremner
@ 2014-01-22 17:30 ` Tomi Ollila
2014-01-23 12:23 ` Round 2: fix for notmuch show segfault David Bremner
0 siblings, 1 reply; 14+ messages in thread
From: Tomi Ollila @ 2014-01-22 17:30 UTC (permalink / raw)
To: David Bremner, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Round 2: fix for notmuch show segfault
2014-01-22 17:30 ` Tomi Ollila
@ 2014-01-23 12:23 ` David Bremner
2014-01-23 12:23 ` [PATCH 1/3] test: add known broken test exit code of notmuch show David Bremner
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: David Bremner @ 2014-01-23 12:23 UTC (permalink / raw)
To: notmuch
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] test: add known broken test exit code of notmuch show
2014-01-23 12:23 ` Round 2: fix for notmuch show segfault David Bremner
@ 2014-01-23 12:23 ` David Bremner
2014-01-23 12:24 ` [PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL David Bremner
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: David Bremner @ 2014-01-23 12:23 UTC (permalink / raw)
To: notmuch
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL
2014-01-23 12:23 ` Round 2: fix for notmuch show segfault David Bremner
2014-01-23 12:23 ` [PATCH 1/3] test: add known broken test exit code of notmuch show David Bremner
@ 2014-01-23 12:24 ` David Bremner
2014-01-24 21:06 ` Jani Nikula
2014-01-23 12:24 ` [PATCH 3/3] notmuch-show: detect xapian exception in query David Bremner
2014-01-23 17:50 ` Round 2: fix for notmuch show segfault Mark Walters
3 siblings, 1 reply; 14+ messages in thread
From: David Bremner @ 2014-01-23 12:24 UTC (permalink / raw)
To: notmuch
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL
2014-01-23 12:24 ` [PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL David Bremner
@ 2014-01-24 21:06 ` Jani Nikula
0 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2014-01-24 21:06 UTC (permalink / raw)
To: David Bremner, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] notmuch-show: detect xapian exception in query
2014-01-23 12:23 ` Round 2: fix for notmuch show segfault David Bremner
2014-01-23 12:23 ` [PATCH 1/3] test: add known broken test exit code of notmuch show David Bremner
2014-01-23 12:24 ` [PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL David Bremner
@ 2014-01-23 12:24 ` David Bremner
2014-01-24 21:12 ` Jani Nikula
2014-01-23 17:50 ` Round 2: fix for notmuch show segfault Mark Walters
3 siblings, 1 reply; 14+ messages in thread
From: David Bremner @ 2014-01-23 12:24 UTC (permalink / raw)
To: notmuch
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] notmuch-show: detect xapian exception in query
2014-01-23 12:24 ` [PATCH 3/3] notmuch-show: detect xapian exception in query David Bremner
@ 2014-01-24 21:12 ` Jani Nikula
2014-01-25 0:41 ` David Bremner
0 siblings, 1 reply; 14+ messages in thread
From: Jani Nikula @ 2014-01-24 21:12 UTC (permalink / raw)
To: David Bremner, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] notmuch-show: detect xapian exception in query
2014-01-24 21:12 ` Jani Nikula
@ 2014-01-25 0:41 ` David Bremner
0 siblings, 0 replies; 14+ messages in thread
From: David Bremner @ 2014-01-25 0:41 UTC (permalink / raw)
To: Jani Nikula, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Round 2: fix for notmuch show segfault
2014-01-23 12:23 ` Round 2: fix for notmuch show segfault David Bremner
` (2 preceding siblings ...)
2014-01-23 12:24 ` [PATCH 3/3] notmuch-show: detect xapian exception in query David Bremner
@ 2014-01-23 17:50 ` Mark Walters
2014-01-24 18:43 ` Tomi Ollila
3 siblings, 1 reply; 14+ messages in thread
From: Mark Walters @ 2014-01-23 17:50 UTC (permalink / raw)
To: David Bremner, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: Round 2: fix for notmuch show segfault
2014-01-23 17:50 ` Round 2: fix for notmuch show segfault Mark Walters
@ 2014-01-24 18:43 ` Tomi Ollila
0 siblings, 0 replies; 14+ messages in thread
From: Tomi Ollila @ 2014-01-24 18:43 UTC (permalink / raw)
To: Mark Walters, David Bremner, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: segfault if notmuch-show query has spurious .. (w/ v0.17)
2014-01-22 2:27 segfault if notmuch-show query has spurious .. (w/ v0.17) Sanjoy Mahajan
2014-01-22 12:40 ` David Bremner
@ 2014-01-25 1:01 ` David Bremner
1 sibling, 0 replies; 14+ messages in thread
From: David Bremner @ 2014-01-25 1:01 UTC (permalink / raw)
To: Sanjoy Mahajan, notmuch
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-01-25 1:01 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-22 2:27 segfault if notmuch-show query has spurious .. (w/ v0.17) Sanjoy Mahajan
2014-01-22 12:40 ` David Bremner
2014-01-22 16:42 ` [PATCH] lib: make notmuch_threads_valid return FALSE when passed NULL David Bremner
2014-01-22 17:30 ` Tomi Ollila
2014-01-23 12:23 ` Round 2: fix for notmuch show segfault David Bremner
2014-01-23 12:23 ` [PATCH 1/3] test: add known broken test exit code of notmuch show David Bremner
2014-01-23 12:24 ` [PATCH 2/3] lib: make notmuch_threads_valid return FALSE when passed NULL David Bremner
2014-01-24 21:06 ` Jani Nikula
2014-01-23 12:24 ` [PATCH 3/3] notmuch-show: detect xapian exception in query David Bremner
2014-01-24 21:12 ` Jani Nikula
2014-01-25 0:41 ` David Bremner
2014-01-23 17:50 ` Round 2: fix for notmuch show segfault Mark Walters
2014-01-24 18:43 ` Tomi Ollila
2014-01-25 1:01 ` segfault if notmuch-show query has spurious .. (w/ v0.17) 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).