Segmentation fault in notmuch search --format=json

Segmentation fault in notmuch search --format=json

[Ben Gamari]

It seems some messages trigger a segmentation fault in
`do_search_threads()`. It appears the problem occurs (at least) when
`authors` is NULL.

    Program received signal SIGSEGV, Segmentation fault.
    0x0000000000415aa3 in json_string (sp=0x646c70, val=0x0) at sprinter-json.c:121
    121	    json_string_len (sp, val, strlen (val));
    (gdb) bt
    #0  0x0000000000415aa3 in json_string (sp=0x646c70, val=0x0)
        at sprinter-json.c:121
    #1  0x000000000041084b in do_search_threads (format=0x646c70, query=0x64bb70, 
        sort=NOTMUCH_SORT_NEWEST_FIRST, output=OUTPUT_SUMMARY, offset=0, limit=-1)
        at notmuch-search.c:131
    #2  0x0000000000411361 in notmuch_search_command (ctx=0x6361b0, argc=3, 
        argv=0x7fffffffdfb0) at notmuch-search.c:405
    #3  0x0000000000409e22 in main (argc=4, argv=0x7fffffffdfa8) at notmuch.c:294
    (gdb) up
    #1  0x000000000041084b in do_search_threads (format=0x646c70, query=0x64bb70, 
        sort=NOTMUCH_SORT_NEWEST_FIRST, output=OUTPUT_SUMMARY, offset=0, limit=-1)
        at notmuch-search.c:131
    131			format->string (format, authors);
    (gdb) list
    126			format->map_key (format, "matched");
    127			format->integer (format, matched);
    128			format->map_key (format, "total");
    129			format->integer (format, total);
    130			format->map_key (format, "authors");
    131			format->string (format, authors);
    132			format->map_key (format, "subject");
    133			format->string (format, subject);
    134		    }
    135	
    (gdb) 

It seems that the message in question was produced by a script I use to
feed RSS feeds into notmuch so while I wouldn't doubt that there are few
cases where `authors` should be NULL, it would be nice if notmuch
handled this case with a bit more grace.

Cheers,

- Ben

Re: Segmentation fault in notmuch search --format=json

[Mark Walters]

On Tue, 07 Aug 2012, Ben Gamari <bgamari.foss@gmail.com> wrote:
> It seems some messages trigger a segmentation fault in
> `do_search_threads()`. It appears the problem occurs (at least) when
> `authors` is NULL.

Hi thanks for the bug report and detailed debugging. I think I can see
the problem and there is a test patch to fix it below, and this does
appear to be a regression.

In json.c the function json_quote_str explicitly checks/allows for a
NULL pointer passed as a string and pretends it is just an empty
string. That behaviour was lost in the move to structured formatters.

A simple fix is to put this check for a null pointer in json_string in
sprinter-json.c which is what this patch does.

Incidentally this is the second time this bug has appeared: 

        commit cacefbf3d6dd5bce0b60b3cdfce29bfa371dfaea
        Author: David Edmondson <dme@dme.org>
        Date:   Tue Apr 6 08:24:00 2010 +0100

            json: Avoid calling strlen(NULL)
                
                    MIME parts may have no filename, which previously resulted in calling
                        strlen(NULL).

so it really might be worth having a test for it!

Finally, I think nothing in json.c is used anymore so perhaps it
 could be removed.


diff --git a/sprinter-json.c b/sprinter-json.c
index c9b6835..0a07790 100644
--- a/sprinter-json.c
+++ b/sprinter-json.c
@@ -118,6 +118,8 @@ json_string_len (struct sprinter *sp, const char *val, size_t len)
 static void
 json_string (struct sprinter *sp, const char *val)
 {
+    if (val == NULL)
+	val = "";
     json_string_len (sp, val, strlen (val));
 }
 

Re: Segmentation fault in notmuch search --format=json

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

On Tue, Aug 07 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> On Tue, 07 Aug 2012, Ben Gamari <bgamari.foss@gmail.com> wrote:
>> It seems some messages trigger a segmentation fault in
>> `do_search_threads()`. It appears the problem occurs (at least) when
>> `authors` is NULL.
>
> Hi thanks for the bug report and detailed debugging. I think I can see
> the problem and there is a test patch to fix it below, and this does
> appear to be a regression.
>
> In json.c the function json_quote_str explicitly checks/allows for a
> NULL pointer passed as a string and pretends it is just an empty
> string. That behaviour was lost in the move to structured formatters.
>
> A simple fix is to put this check for a null pointer in json_string in
> sprinter-json.c which is what this patch does.

Thanks Mark!  I was experiencing the same problem and this fixed the
issue before I even got a chance to respond.  This seems like a fine
solution.

> Incidentally this is the second time this bug has appeared: 
>
>         commit cacefbf3d6dd5bce0b60b3cdfce29bfa371dfaea
>         Author: David Edmondson <dme@dme.org>
>         Date:   Tue Apr 6 08:24:00 2010 +0100
>
>             json: Avoid calling strlen(NULL)
>                 
>                     MIME parts may have no filename, which previously resulted in calling
>                         strlen(NULL).
>
> so it really might be worth having a test for it!

Indeed!  I think the problematic email in this case was one with no
subject.

> Finally, I think nothing in json.c is used anymore so perhaps it
>  could be removed.

Agreed.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Segmentation fault in notmuch search --format=json

[Austin Clements]

Quoth Mark Walters on Aug 07 at  8:07 am:
> On Tue, 07 Aug 2012, Ben Gamari <bgamari.foss@gmail.com> wrote:
> > It seems some messages trigger a segmentation fault in
> > `do_search_threads()`. It appears the problem occurs (at least) when
> > `authors` is NULL.
> 
> Hi thanks for the bug report and detailed debugging. I think I can see
> the problem and there is a test patch to fix it below, and this does
> appear to be a regression.
> 
> In json.c the function json_quote_str explicitly checks/allows for a
> NULL pointer passed as a string and pretends it is just an empty
> string. That behaviour was lost in the move to structured formatters.
> 
> A simple fix is to put this check for a null pointer in json_string in
> sprinter-json.c which is what this patch does.
> 
> Incidentally this is the second time this bug has appeared: 
> 
>         commit cacefbf3d6dd5bce0b60b3cdfce29bfa371dfaea
>         Author: David Edmondson <dme@dme.org>
>         Date:   Tue Apr 6 08:24:00 2010 +0100
> 
>             json: Avoid calling strlen(NULL)
>                 
>                     MIME parts may have no filename, which previously resulted in calling
>                         strlen(NULL).
> 
> so it really might be worth having a test for it!
> 
> Finally, I think nothing in json.c is used anymore so perhaps it
>  could be removed.

LGTM.  We'll want to do something similar for text_string and, of
course, update the sprinter doc comments.

> diff --git a/sprinter-json.c b/sprinter-json.c
> index c9b6835..0a07790 100644
> --- a/sprinter-json.c
> +++ b/sprinter-json.c
> @@ -118,6 +118,8 @@ json_string_len (struct sprinter *sp, const char *val, size_t len)
>  static void
>  json_string (struct sprinter *sp, const char *val)
>  {
> +    if (val == NULL)
> +	val = "";
>      json_string_len (sp, val, strlen (val));
>  }
>  

[PATCH] sprinters: bugfix when NULL passed for a string.

[Mark Walters]

The string function in a sprinter may be called with a NULL string
pointer (eg if a header is absent). This causes a segfault. We fix
this by checking for a null pointer in the string functions and update
the sprinter documentation.

At the moment some output when format=text is done directly rather than
via an sprinter: in that case a null pointer is passed to printf or
similar and a "(null)" appears in the output. That behaviour is not
changed in this patch.
---

This could really do with some tests (it is the second time this type of
bug has occurred). To be considered as a message by notmuch new a file
needs at least one of a From: Subject: or To: header. Thus we should
have three messages each of which just contains that single header (and
nothing else) and check that search and show work as expected. 



 sprinter-json.c |    2 ++
 sprinter-text.c |    2 ++
 sprinter.h      |    4 +++-
 3 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/sprinter-json.c b/sprinter-json.c
index c9b6835..0a07790 100644
--- a/sprinter-json.c
+++ b/sprinter-json.c
@@ -118,6 +118,8 @@ json_string_len (struct sprinter *sp, const char *val, size_t len)
 static void
 json_string (struct sprinter *sp, const char *val)
 {
+    if (val == NULL)
+	val = "";
     json_string_len (sp, val, strlen (val));
 }
 
diff --git a/sprinter-text.c b/sprinter-text.c
index dfa54b5..10343be 100644
--- a/sprinter-text.c
+++ b/sprinter-text.c
@@ -38,6 +38,8 @@ text_string_len (struct sprinter *sp, const char *val, size_t len)
 static void
 text_string (struct sprinter *sp, const char *val)
 {
+    if (val == NULL)
+	val = "";
     text_string_len (sp, val, strlen (val));
 }
 
diff --git a/sprinter.h b/sprinter.h
index 5f43175..912a526 100644
--- a/sprinter.h
+++ b/sprinter.h
@@ -27,7 +27,9 @@ typedef struct sprinter {
      * a list or map, followed or preceded by separators).  For string
      * and string_len, the char * must be UTF-8 encoded.  string_len
      * allows non-terminated strings and strings with embedded NULs
-     * (though the handling of the latter is format-dependent).
+     * (though the handling of the latter is format-dependent). For
+     * string (but not string_len) the string pointer passed may be
+     * NULL.
      */
     void (*string) (struct sprinter *, const char *);
     void (*string_len) (struct sprinter *, const char *, size_t);
-- 
1.7.9.1


H

Re: [PATCH] sprinters: bugfix when NULL passed for a string.

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

On Tue, Aug 07 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> The string function in a sprinter may be called with a NULL string
> pointer (eg if a header is absent). This causes a segfault. We fix
> this by checking for a null pointer in the string functions and update
> the sprinter documentation.
>
> At the moment some output when format=text is done directly rather than
> via an sprinter: in that case a null pointer is passed to printf or
> similar and a "(null)" appears in the output. That behaviour is not
> changed in this patch.
> ---
>
> This could really do with some tests (it is the second time this type of
> bug has occurred). To be considered as a message by notmuch new a file
> needs at least one of a From: Subject: or To: header. Thus we should
> have three messages each of which just contains that single header (and
> nothing else) and check that search and show work as expected. 

Hey, Mark.  Thanks for working on this.

I was wondering if we should distinguish between the header being
absent, and having a null value.  It looks like the idea here is to
output an empty string for the value in all of these cases.  But should
we output the field at all if the actual header isn't there?  In other
words, I can imagine three scenarios:

Header: value
Header:        --> "Header": ""
no header     

At the moment these would be output as:

"Header": "value"
"Header": ""
"Header": ""

Where as I could imagine we could instead do:

"Header": "value"
"Header": ""
no output

Maybe that would be too complicated or break the output spec to much?
If it's too complicated to do the later, then I'm fine with this
solution as is.

I definitely agree we need tests for this.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] sprinters: bugfix when NULL passed for a string.

[Austin Clements]

LGTM.

This won't commute with [0], since that introduces broken tests that are
fixed by this patch.

I think we should remove the fields in the JSON header object for
missing headers (except perhaps From and Date, if those really are
mandatory headers), but I think we should do that after the freeze,
since it might affect frontends.  It probably makes sense to add an
Emacs test or two to the tests in [0].

[0] id:"1344389313-7886-1-git-send-email-amdragon@mit.edu"

On Tue, 07 Aug 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> The string function in a sprinter may be called with a NULL string
> pointer (eg if a header is absent). This causes a segfault. We fix
> this by checking for a null pointer in the string functions and update
> the sprinter documentation.
>
> At the moment some output when format=text is done directly rather than
> via an sprinter: in that case a null pointer is passed to printf or
> similar and a "(null)" appears in the output. That behaviour is not
> changed in this patch.
> ---
>
> This could really do with some tests (it is the second time this type of
> bug has occurred). To be considered as a message by notmuch new a file
> needs at least one of a From: Subject: or To: header. Thus we should
> have three messages each of which just contains that single header (and
> nothing else) and check that search and show work as expected. 
>
>
>
>  sprinter-json.c |    2 ++
>  sprinter-text.c |    2 ++
>  sprinter.h      |    4 +++-
>  3 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/sprinter-json.c b/sprinter-json.c
> index c9b6835..0a07790 100644
> --- a/sprinter-json.c
> +++ b/sprinter-json.c
> @@ -118,6 +118,8 @@ json_string_len (struct sprinter *sp, const char *val, size_t len)
>  static void
>  json_string (struct sprinter *sp, const char *val)
>  {
> +    if (val == NULL)
> +	val = "";
>      json_string_len (sp, val, strlen (val));
>  }
>  
> diff --git a/sprinter-text.c b/sprinter-text.c
> index dfa54b5..10343be 100644
> --- a/sprinter-text.c
> +++ b/sprinter-text.c
> @@ -38,6 +38,8 @@ text_string_len (struct sprinter *sp, const char *val, size_t len)
>  static void
>  text_string (struct sprinter *sp, const char *val)
>  {
> +    if (val == NULL)
> +	val = "";
>      text_string_len (sp, val, strlen (val));
>  }
>  
> diff --git a/sprinter.h b/sprinter.h
> index 5f43175..912a526 100644
> --- a/sprinter.h
> +++ b/sprinter.h
> @@ -27,7 +27,9 @@ typedef struct sprinter {
>       * a list or map, followed or preceded by separators).  For string
>       * and string_len, the char * must be UTF-8 encoded.  string_len
>       * allows non-terminated strings and strings with embedded NULs
> -     * (though the handling of the latter is format-dependent).
> +     * (though the handling of the latter is format-dependent). For
> +     * string (but not string_len) the string pointer passed may be
> +     * NULL.
>       */
>      void (*string) (struct sprinter *, const char *);
>      void (*string_len) (struct sprinter *, const char *, size_t);
> -- 
> 1.7.9.1
>
>
> H
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] sprinters: bugfix when NULL passed for a string.

[Mark Walters]

On Wed, 08 Aug 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> LGTM.
>
> This won't commute with [0], since that introduces broken tests that are
> fixed by this patch.

Yes: I guess the tidiest might be for me to send a version of my patch
which marks these tests fixed so it can be applied after this.

> I think we should remove the fields in the JSON header object for
> missing headers (except perhaps From and Date, if those really are
> mandatory headers), but I think we should do that after the freeze,
> since it might affect frontends.  

I agree with you and Jamie on this. I think it is a `feature' rather
than a bugfix (the current patch just restores the output to what it was
before the sprinter changes it) so agree it should be after the
freeze. We could deprecate passing NULL to sprinter string functions
(possibly keep the check but fprintf a warning?)

For the From/Date headers see http://www.ietf.org/rfc/rfc2822.txt
section 3.6 for details:

   The only required header fields are the origination date field and
   the originator address field(s).  All other header fields are
   syntactically optional.

and origination date field means a Date: header, and originator
address field means a From: header. However, I don't think an empty
string is valid for either of these so we can't sensibly output
something standards compliant. Thus I would go for following the
original message and omit any fields not present in it.

> It probably makes sense to add an Emacs test or two to the tests in [0].

This seems like a good idea.

Best wishes

Mark


> [0] id:"1344389313-7886-1-git-send-email-amdragon@mit.edu"
>
> On Tue, 07 Aug 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>> The string function in a sprinter may be called with a NULL string
>> pointer (eg if a header is absent). This causes a segfault. We fix
>> this by checking for a null pointer in the string functions and update
>> the sprinter documentation.
>>
>> At the moment some output when format=text is done directly rather than
>> via an sprinter: in that case a null pointer is passed to printf or
>> similar and a "(null)" appears in the output. That behaviour is not
>> changed in this patch.
>> ---
>>
>> This could really do with some tests (it is the second time this type of
>> bug has occurred). To be considered as a message by notmuch new a file
>> needs at least one of a From: Subject: or To: header. Thus we should
>> have three messages each of which just contains that single header (and
>> nothing else) and check that search and show work as expected. 
>>
>>
>>
>>  sprinter-json.c |    2 ++
>>  sprinter-text.c |    2 ++
>>  sprinter.h      |    4 +++-
>>  3 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/sprinter-json.c b/sprinter-json.c
>> index c9b6835..0a07790 100644
>> --- a/sprinter-json.c
>> +++ b/sprinter-json.c
>> @@ -118,6 +118,8 @@ json_string_len (struct sprinter *sp, const char *val, size_t len)
>>  static void
>>  json_string (struct sprinter *sp, const char *val)
>>  {
>> +    if (val == NULL)
>> +	val = "";
>>      json_string_len (sp, val, strlen (val));
>>  }
>>  
>> diff --git a/sprinter-text.c b/sprinter-text.c
>> index dfa54b5..10343be 100644
>> --- a/sprinter-text.c
>> +++ b/sprinter-text.c
>> @@ -38,6 +38,8 @@ text_string_len (struct sprinter *sp, const char *val, size_t len)
>>  static void
>>  text_string (struct sprinter *sp, const char *val)
>>  {
>> +    if (val == NULL)
>> +	val = "";
>>      text_string_len (sp, val, strlen (val));
>>  }
>>  
>> diff --git a/sprinter.h b/sprinter.h
>> index 5f43175..912a526 100644
>> --- a/sprinter.h
>> +++ b/sprinter.h
>> @@ -27,7 +27,9 @@ typedef struct sprinter {
>>       * a list or map, followed or preceded by separators).  For string
>>       * and string_len, the char * must be UTF-8 encoded.  string_len
>>       * allows non-terminated strings and strings with embedded NULs
>> -     * (though the handling of the latter is format-dependent).
>> +     * (though the handling of the latter is format-dependent). For
>> +     * string (but not string_len) the string pointer passed may be
>> +     * NULL.
>>       */
>>      void (*string) (struct sprinter *, const char *);
>>      void (*string_len) (struct sprinter *, const char *, size_t);
>> -- 
>> 1.7.9.1
>>
>>
>> H
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] sprinters: bugfix when NULL passed for a string.

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

On Wed, Aug 08 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> For the From/Date headers see http://www.ietf.org/rfc/rfc2822.txt
> section 3.6 for details:
>
>    The only required header fields are the origination date field and
>    the originator address field(s).  All other header fields are
>    syntactically optional.
>
> and origination date field means a Date: header, and originator
> address field means a From: header. However, I don't think an empty
> string is valid for either of these so we can't sensibly output
> something standards compliant. Thus I would go for following the
> original message and omit any fields not present in it.

I agree.  Let's not pretend or imply something is valid when it's not.
The output should reflect the actual content of the message as much as
possible.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

[PATCH] test: Add test for messages with missing headers

[Austin Clements]

Currently the JSON tests for search and show are broken because
notmuch attempts to dereference a NULL pointer.
---
Things to bikeshed:

* Should we include From and Subject in the headers object when there
  are no from or subject headers?  Currently the schema says that
  everything but those two and "Date" is optional (indeed, "To" is
  missing from the second message) but that was just post facto
  standardization.

* How should we format expected JSON in the test suite, now that we
  can format it however we want?  Here I've just pasted in the result
  of python -mjson.tool.  While that was very easy and the result is
  quite readable, it's the antithesis of compact and the keys have
  been alphabetized.

 test/missing-headers |  162 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test/notmuch-test    |    1 +
 2 files changed, 163 insertions(+)
 create mode 100755 test/missing-headers

diff --git a/test/missing-headers b/test/missing-headers
new file mode 100755
index 0000000..744c04e
--- /dev/null
+++ b/test/missing-headers
@@ -0,0 +1,162 @@
+#!/usr/bin/env bash
+test_description='messages with missing headers'
+. ./test-lib.sh
+
+# Notmuch requires at least one of from, subject, or to or it will
+# ignore the file.  Generate two messages so that together they cover
+# all possible missing headers.  We also give one of the messages a
+# date to ensure stable result ordering.
+
+cat <<EOF > "${MAIL_DIR}/msg-2"
+To: Notmuch Test Suite <test_suite@notmuchmail.org>
+Date: Fri, 05 Jan 2001 15:43:57 +0000
+
+Body
+EOF
+
+cat <<EOF > "${MAIL_DIR}/msg-1"
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+
+Body
+EOF
+
+NOTMUCH_NEW
+
+test_begin_subtest "Search: text"
+output=$(notmuch search '*' | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+thread:XXX   2001-01-05 [1/1] (null);  (inbox unread)
+thread:XXX   1970-01-01 [1/1] Notmuch Test Suite;  (inbox unread)"
+
+test_begin_subtest "Search: json"
+test_subtest_known_broken
+output=$(notmuch search --format=json '*' | notmuch_search_sanitize)
+test_expect_equal_json "$output" '
+[
+    {
+        "authors": "",
+        "date_relative": "2001-01-05",
+        "matched": 1,
+        "subject": "",
+        "tags": [
+            "inbox",
+            "unread"
+        ],
+        "thread": "XXX",
+        "timestamp": 978709437,
+        "total": 1
+    },
+    {
+        "authors": "Notmuch Test Suite",
+        "date_relative": "1970-01-01",
+        "matched": 1,
+        "subject": "",
+        "tags": [
+            "inbox",
+            "unread"
+        ],
+        "thread": "XXX",
+        "timestamp": 0,
+        "total": 1
+    }
+]'
+
+test_begin_subtest "Show: text"
+output=$(notmuch show '*')
+test_expect_equal "$output" "\
+\fmessage{ id:notmuch-sha1-7a6e4eac383ef958fcd3ebf2143db71b8ff01161 depth:0 match:1 excluded:0 filename:/tmp/nmtest/tmp.missing-headers/mail/msg-2
+\fheader{
+ (2001-01-05) (inbox unread)
+Subject: (null)
+From: (null)
+To: Notmuch Test Suite <test_suite@notmuchmail.org>
+Date: Fri, 05 Jan 2001 15:43:57 +0000
+\fheader}
+\fbody{
+\fpart{ ID: 1, Content-type: text/plain
+Body
+\fpart}
+\fbody}
+\fmessage}
+\fmessage{ id:notmuch-sha1-ca55943aff7a72baf2ab21fa74fab3d632401334 depth:0 match:1 excluded:0 filename:/tmp/nmtest/tmp.missing-headers/mail/msg-1
+\fheader{
+Notmuch Test Suite <test_suite@notmuchmail.org> (1970-01-01) (inbox unread)
+Subject: (null)
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+Date: Thu, 01 Jan 1970 00:00:00 +0000
+\fheader}
+\fbody{
+\fpart{ ID: 1, Content-type: text/plain
+Body
+\fpart}
+\fbody}
+\fmessage}"
+
+test_begin_subtest "Show: json"
+test_subtest_known_broken
+output=$(notmuch show --format=json '*' | notmuch_json_show_sanitize)
+test_expect_equal_json "$output" '
+[
+    [
+        [
+            {
+                "body": [
+                    {
+                        "content": "Body\n",
+                        "content-type": "text/plain",
+                        "id": 1
+                    }
+                ],
+                "date_relative": "2001-01-05",
+                "excluded": false,
+                "filename": "YYYYY",
+                "headers": {
+                    "Date": "Fri, 05 Jan 2001 15:43:57 +0000",
+                    "From": "",
+                    "Subject": "",
+                    "To": "Notmuch Test Suite <test_suite@notmuchmail.org>"
+                },
+                "id": "XXXXX",
+                "match": true,
+                "tags": [
+                    "inbox",
+                    "unread"
+                ],
+                "timestamp": 978709437
+            },
+            []
+        ]
+    ],
+    [
+        [
+            {
+                "body": [
+                    {
+                        "content": "Body\n",
+                        "content-type": "text/plain",
+                        "id": 1
+                    }
+                ],
+                "date_relative": "1970-01-01",
+                "excluded": false,
+                "filename": "YYYYY",
+                "headers": {
+                    "Date": "Thu, 01 Jan 1970 00:00:00 +0000",
+                    "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
+                    "Subject": ""
+                },
+                "id": "XXXXX",
+                "match": true,
+                "tags": [
+                    "inbox",
+                    "unread"
+                ],
+                "timestamp": 0
+            },
+            []
+        ]
+    ]
+]'
+
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index ea39dfc..cc732c3 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -59,6 +59,7 @@ TESTS="
   emacs-address-cleaning
   emacs-hello
   emacs-show
+  missing-headers
 "
 TESTS=${NOTMUCH_TESTS:=$TESTS}
 
-- 
1.7.10

[PATCH v2] test: Add test for messages with missing headers

[Austin Clements]

Currently the JSON tests for search and show are broken because
notmuch attempts to dereference a NULL pointer.
---
This version fixes the "Show: text" test so that it sanitize its
output and doesn't hard-code my test paths.

 test/missing-headers |  162 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test/notmuch-test    |    1 +
 2 files changed, 163 insertions(+)
 create mode 100755 test/missing-headers

diff --git a/test/missing-headers b/test/missing-headers
new file mode 100755
index 0000000..e79f922
--- /dev/null
+++ b/test/missing-headers
@@ -0,0 +1,162 @@
+#!/usr/bin/env bash
+test_description='messages with missing headers'
+. ./test-lib.sh
+
+# Notmuch requires at least one of from, subject, or to or it will
+# ignore the file.  Generate two messages so that together they cover
+# all possible missing headers.  We also give one of the messages a
+# date to ensure stable result ordering.
+
+cat <<EOF > "${MAIL_DIR}/msg-2"
+To: Notmuch Test Suite <test_suite@notmuchmail.org>
+Date: Fri, 05 Jan 2001 15:43:57 +0000
+
+Body
+EOF
+
+cat <<EOF > "${MAIL_DIR}/msg-1"
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+
+Body
+EOF
+
+NOTMUCH_NEW
+
+test_begin_subtest "Search: text"
+output=$(notmuch search '*' | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+thread:XXX   2001-01-05 [1/1] (null);  (inbox unread)
+thread:XXX   1970-01-01 [1/1] Notmuch Test Suite;  (inbox unread)"
+
+test_begin_subtest "Search: json"
+test_subtest_known_broken
+output=$(notmuch search --format=json '*' | notmuch_search_sanitize)
+test_expect_equal_json "$output" '
+[
+    {
+        "authors": "",
+        "date_relative": "2001-01-05",
+        "matched": 1,
+        "subject": "",
+        "tags": [
+            "inbox",
+            "unread"
+        ],
+        "thread": "XXX",
+        "timestamp": 978709437,
+        "total": 1
+    },
+    {
+        "authors": "Notmuch Test Suite",
+        "date_relative": "1970-01-01",
+        "matched": 1,
+        "subject": "",
+        "tags": [
+            "inbox",
+            "unread"
+        ],
+        "thread": "XXX",
+        "timestamp": 0,
+        "total": 1
+    }
+]'
+
+test_begin_subtest "Show: text"
+output=$(notmuch show '*' | notmuch_show_sanitize)
+test_expect_equal "$output" "\
+\fmessage{ id:notmuch-sha1-7a6e4eac383ef958fcd3ebf2143db71b8ff01161 depth:0 match:1 excluded:0 filename:/XXX/mail/msg-2
+\fheader{
+ (2001-01-05) (inbox unread)
+Subject: (null)
+From: (null)
+To: Notmuch Test Suite <test_suite@notmuchmail.org>
+Date: Fri, 05 Jan 2001 15:43:57 +0000
+\fheader}
+\fbody{
+\fpart{ ID: 1, Content-type: text/plain
+Body
+\fpart}
+\fbody}
+\fmessage}
+\fmessage{ id:notmuch-sha1-ca55943aff7a72baf2ab21fa74fab3d632401334 depth:0 match:1 excluded:0 filename:/XXX/mail/msg-1
+\fheader{
+Notmuch Test Suite <test_suite@notmuchmail.org> (1970-01-01) (inbox unread)
+Subject: (null)
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+Date: Thu, 01 Jan 1970 00:00:00 +0000
+\fheader}
+\fbody{
+\fpart{ ID: 1, Content-type: text/plain
+Body
+\fpart}
+\fbody}
+\fmessage}"
+
+test_begin_subtest "Show: json"
+test_subtest_known_broken
+output=$(notmuch show --format=json '*' | notmuch_json_show_sanitize)
+test_expect_equal_json "$output" '
+[
+    [
+        [
+            {
+                "body": [
+                    {
+                        "content": "Body\n",
+                        "content-type": "text/plain",
+                        "id": 1
+                    }
+                ],
+                "date_relative": "2001-01-05",
+                "excluded": false,
+                "filename": "YYYYY",
+                "headers": {
+                    "Date": "Fri, 05 Jan 2001 15:43:57 +0000",
+                    "From": "",
+                    "Subject": "",
+                    "To": "Notmuch Test Suite <test_suite@notmuchmail.org>"
+                },
+                "id": "XXXXX",
+                "match": true,
+                "tags": [
+                    "inbox",
+                    "unread"
+                ],
+                "timestamp": 978709437
+            },
+            []
+        ]
+    ],
+    [
+        [
+            {
+                "body": [
+                    {
+                        "content": "Body\n",
+                        "content-type": "text/plain",
+                        "id": 1
+                    }
+                ],
+                "date_relative": "1970-01-01",
+                "excluded": false,
+                "filename": "YYYYY",
+                "headers": {
+                    "Date": "Thu, 01 Jan 1970 00:00:00 +0000",
+                    "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
+                    "Subject": ""
+                },
+                "id": "XXXXX",
+                "match": true,
+                "tags": [
+                    "inbox",
+                    "unread"
+                ],
+                "timestamp": 0
+            },
+            []
+        ]
+    ]
+]'
+
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index ea39dfc..cc732c3 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -59,6 +59,7 @@ TESTS="
   emacs-address-cleaning
   emacs-hello
   emacs-show
+  missing-headers
 "
 TESTS=${NOTMUCH_TESTS:=$TESTS}
 
-- 
1.7.10

[PATCH] sprinters: bugfix when NULL passed for a string.

[Mark Walters]

The string function in a sprinter may be called with a NULL string
pointer (eg if a header is absent). This causes a segfault. We fix
this by checking for a null pointer in the string functions and update
the sprinter documentation.

At the moment some output when format=text is done directly rather than
via an sprinter: in that case a null pointer is passed to printf or
similar and a "(null)" appears in the output. That behaviour is not
changed in this patch.
---

This is the same as id:"874noe1o0r.fsf@qmul.ac.uk" except for being
based on top of the test in the parent message, and marking the broken
test fixed.

Best wishes

Mark

 sprinter-json.c      |    2 ++
 sprinter-text.c      |    2 ++
 sprinter.h           |    4 +++-
 test/missing-headers |    2 --
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/sprinter-json.c b/sprinter-json.c
index c9b6835..0a07790 100644
--- a/sprinter-json.c
+++ b/sprinter-json.c
@@ -118,6 +118,8 @@ json_string_len (struct sprinter *sp, const char *val, size_t len)
 static void
 json_string (struct sprinter *sp, const char *val)
 {
+    if (val == NULL)
+	val = "";
     json_string_len (sp, val, strlen (val));
 }
 
diff --git a/sprinter-text.c b/sprinter-text.c
index dfa54b5..10343be 100644
--- a/sprinter-text.c
+++ b/sprinter-text.c
@@ -38,6 +38,8 @@ text_string_len (struct sprinter *sp, const char *val, size_t len)
 static void
 text_string (struct sprinter *sp, const char *val)
 {
+    if (val == NULL)
+	val = "";
     text_string_len (sp, val, strlen (val));
 }
 
diff --git a/sprinter.h b/sprinter.h
index 5f43175..912a526 100644
--- a/sprinter.h
+++ b/sprinter.h
@@ -27,7 +27,9 @@ typedef struct sprinter {
      * a list or map, followed or preceded by separators).  For string
      * and string_len, the char * must be UTF-8 encoded.  string_len
      * allows non-terminated strings and strings with embedded NULs
-     * (though the handling of the latter is format-dependent).
+     * (though the handling of the latter is format-dependent). For
+     * string (but not string_len) the string pointer passed may be
+     * NULL.
      */
     void (*string) (struct sprinter *, const char *);
     void (*string_len) (struct sprinter *, const char *, size_t);
diff --git a/test/missing-headers b/test/missing-headers
index e79f922..f14b878 100755
--- a/test/missing-headers
+++ b/test/missing-headers
@@ -29,7 +29,6 @@ thread:XXX   2001-01-05 [1/1] (null);  (inbox unread)
 thread:XXX   1970-01-01 [1/1] Notmuch Test Suite;  (inbox unread)"
 
 test_begin_subtest "Search: json"
-test_subtest_known_broken
 output=$(notmuch search --format=json '*' | notmuch_search_sanitize)
 test_expect_equal_json "$output" '
 [
@@ -93,7 +92,6 @@ Body
 \fmessage}"
 
 test_begin_subtest "Show: json"
-test_subtest_known_broken
 output=$(notmuch show --format=json '*' | notmuch_json_show_sanitize)
 test_expect_equal_json "$output" '
 [
-- 
1.7.9.1

Re: [PATCH] sprinters: bugfix when NULL passed for a string.

[Austin Clements]

LGTM!

Quoth Mark Walters on Aug 08 at 10:23 pm:
> 
> The string function in a sprinter may be called with a NULL string
> pointer (eg if a header is absent). This causes a segfault. We fix
> this by checking for a null pointer in the string functions and update
> the sprinter documentation.
> 
> At the moment some output when format=text is done directly rather than
> via an sprinter: in that case a null pointer is passed to printf or
> similar and a "(null)" appears in the output. That behaviour is not
> changed in this patch.
> ---
> 
> This is the same as id:"874noe1o0r.fsf@qmul.ac.uk" except for being
> based on top of the test in the parent message, and marking the broken
> test fixed.
> 
> Best wishes
> 
> Mark
> 
>  sprinter-json.c      |    2 ++
>  sprinter-text.c      |    2 ++
>  sprinter.h           |    4 +++-
>  test/missing-headers |    2 --
>  4 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/sprinter-json.c b/sprinter-json.c
> index c9b6835..0a07790 100644
> --- a/sprinter-json.c
> +++ b/sprinter-json.c
> @@ -118,6 +118,8 @@ json_string_len (struct sprinter *sp, const char *val, size_t len)
>  static void
>  json_string (struct sprinter *sp, const char *val)
>  {
> +    if (val == NULL)
> +	val = "";
>      json_string_len (sp, val, strlen (val));
>  }
>  
> diff --git a/sprinter-text.c b/sprinter-text.c
> index dfa54b5..10343be 100644
> --- a/sprinter-text.c
> +++ b/sprinter-text.c
> @@ -38,6 +38,8 @@ text_string_len (struct sprinter *sp, const char *val, size_t len)
>  static void
>  text_string (struct sprinter *sp, const char *val)
>  {
> +    if (val == NULL)
> +	val = "";
>      text_string_len (sp, val, strlen (val));
>  }
>  
> diff --git a/sprinter.h b/sprinter.h
> index 5f43175..912a526 100644
> --- a/sprinter.h
> +++ b/sprinter.h
> @@ -27,7 +27,9 @@ typedef struct sprinter {
>       * a list or map, followed or preceded by separators).  For string
>       * and string_len, the char * must be UTF-8 encoded.  string_len
>       * allows non-terminated strings and strings with embedded NULs
> -     * (though the handling of the latter is format-dependent).
> +     * (though the handling of the latter is format-dependent). For
> +     * string (but not string_len) the string pointer passed may be
> +     * NULL.
>       */
>      void (*string) (struct sprinter *, const char *);
>      void (*string_len) (struct sprinter *, const char *, size_t);
> diff --git a/test/missing-headers b/test/missing-headers
> index e79f922..f14b878 100755
> --- a/test/missing-headers
> +++ b/test/missing-headers
> @@ -29,7 +29,6 @@ thread:XXX   2001-01-05 [1/1] (null);  (inbox unread)
>  thread:XXX   1970-01-01 [1/1] Notmuch Test Suite;  (inbox unread)"
>  
>  test_begin_subtest "Search: json"
> -test_subtest_known_broken
>  output=$(notmuch search --format=json '*' | notmuch_search_sanitize)
>  test_expect_equal_json "$output" '
>  [
> @@ -93,7 +92,6 @@ Body
>  \fmessage}"
>  
>  test_begin_subtest "Show: json"
> -test_subtest_known_broken
>  output=$(notmuch show --format=json '*' | notmuch_json_show_sanitize)
>  test_expect_equal_json "$output" '
>  [

Re: [PATCH] sprinters: bugfix when NULL passed for a string.

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> The string function in a sprinter may be called with a NULL string
> pointer (eg if a header is absent). This causes a segfault. We fix
> this by checking for a null pointer in the string functions 

pushed, 

d