Re: [PATCH] sprinters: bugfix when NULL passed for a string.
Mark Walters markwalters1...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] sprinters: bugfix when NULL passed for a string.
On Wed, 08 Aug 2012, Austin Clements amdra...@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-amdra...@mit.edu
On Tue, 07 Aug 2012, Mark Walters markwalters1...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] sprinters: bugfix when NULL passed for a string.
On Wed, Aug 08 2012, Mark Walters markwalters1...@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. pgp8Tb3i46WCX.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] sprinters: bugfix when NULL passed for a string.
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@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
message}
test_begin_subtest Show: json
-test_subtest_known_broken
output=$(notmuch show --format=json '*' | notmuch_json_show_sanitize)
test_expect_equal_json $output '
[
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] sprinters: bugfix when NULL passed for a string.
On Tue, Aug 07 2012, Mark Walters markwalters1...@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. pgpwRxLzAxTqr.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] sprinters: bugfix when NULL passed for a string.
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-amdra...@mit.edu
On Tue, 07 Aug 2012, Mark Walters markwalters1...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
