Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display
Hi Bremner--
thanks for the review!
On Sat 2019-08-03 12:15:30 -0300, David Bremner wrote:
> Daniel Kahn Gillmor writes:
>
>> +ret = true;
>> +for (int i = 0; i < g_mime_header_list_get_count
>> (legacy_display_headers); i++) {
>> +GMimeHeader *dh = g_mime_header_list_get_header_at
>> (legacy_display_headers, i);
>> +if (dh == NULL) {
>> +ret = false;
>> +break;
>> +}
>
> I can live with the use of break if you think it's superior, but I think
> the idiom of "goto DONE" is more common in the notmuch codebase. I
> personally always have think about the semantics of "break" and
> "continue" in C pretty carefully.
i thought i was the only one who got confused between "break" and
"continue"! I will convert to goto DONE, i agree it's more readable.
>> +if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value
>> (ph))) {
>> +ret = false;
>> +break;
>> +}
>
> It's not really clear to me what kind of "invalid" causes
> g_mime_header_get_value to return NULL. Maybe this strcmp should be
> guarded against that?
i think it's impossible in the current implementation for this to go
wrong, since we've already got a GMimeHeader object from an existing
block of headers, but i'll add some protection just in case GMime
changes its implementation or some fuzzer constructs a truly devious
not-quite-RFC-822 input.
--dkg
signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display
Daniel Kahn Gillmor writes:
> + ret = true;
> + for (int i = 0; i < g_mime_header_list_get_count
> (legacy_display_headers); i++) {
> + GMimeHeader *dh = g_mime_header_list_get_header_at
> (legacy_display_headers, i);
> + if (dh == NULL) {
> + ret = false;
> + break;
> + }
I can live with the use of break if you think it's superior, but I think
the idiom of "goto DONE" is more common in the notmuch codebase. I
personally always have think about the semantics of "break" and
"continue" in C pretty carefully.
> + if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value
> (ph))) {
> + ret = false;
> + break;
> + }
It's not really clear to me what kind of "invalid" causes
g_mime_header_get_value to return NULL. Maybe this strcmp should be
guarded against that?
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display
On Mon 2019-06-24 20:02:13 -0700, William Casarin wrote:
> dkg wrote:
>> +if ((protected_headers = g_mime_object_get_header_list (payload),
>> protected_headers) &&
>> +(legacy_display = GMIME_TEXT_PART (first), legacy_display) &&
>> +(legacy_display_header_text = g_mime_text_part_get_text
>> (legacy_display), legacy_display_header_text) &&
>> +(stream = g_mime_stream_mem_new_with_buffer
>> (legacy_display_header_text, strlen (legacy_display_header_text)), stream) &&
>> +(g_mime_stream_write (stream, "\r\n\r\n", 4) == 4) &&
>> +(g_mime_stream_seek (stream, 0, GMIME_STREAM_SEEK_SET) == 0) &&
>> +(parser = g_mime_parser_new_with_stream (stream), parser) &&
>> +(legacy_header_object = g_mime_parser_construct_part (parser, NULL),
>> legacy_header_object) &&
>> +(legacy_display_headers = g_mime_object_get_header_list
>> (legacy_header_object), legacy_display_headers)) {
>> +/* walk through legacy_display_headers, comparing them against
>
> This may be a noob question, but why the comma operators after the
> assignment expressions? Wouldn't they evaluate to the same thing?
Yes, they do evaluate to the same thing.
But A sensible compiler will warn when it sees "if (foo = bar)" because
most people who write that actually mean "if (foo == bar)".
So using the comma operator signals to the compiler "yes, I meant to
test the result of this particular assignment, and it is not just a
buggy attempt at a comparison".
--dkg
PS if you leave out the comma, gcc's actual warning with -Wall is
disappointingly opaque:
warning: suggest parentheses around assignment used as truth value
[-Wparentheses]
This suggests that you could also work around it by using a construct
like"if ((foo = bar))". But i think that is harder to read, and
doesn't really describe what the intent is as well.
As Hal Abelson said, "Programs must be written for people to read,
and only incidentally for machines to execute."
signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display
Daniel Kahn Gillmor writes:
> This is a utility function designed to make it easier to
> "fast-forward" past a legacy-display part associated with a
> cryptographic envelope, and show the user the intended message body.
>
> The bulk of the ugliness in here is in the test function
> _notmuch_crypto_payload_has_legacy_display, which tests all of the
> things we'd expect to be true in a a cryptographic payload that
> contains a legacy display part.
>
> Signed-off-by: Daniel Kahn Gillmor
> ---
> util/repair.c | 98 +++
> util/repair.h | 17 +
> 2 files changed, 115 insertions(+)
>
> diff --git a/util/repair.c b/util/repair.c
> index f91c1244..0809f7b4 100644
> --- a/util/repair.c
> +++ b/util/repair.c
> @@ -18,4 +18,102 @@
> * Authors: Daniel Kahn Gillmor
> */
>
> +#include
> #include "repair.h"
> +
> +
> +static bool
> +_notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
> +{
> +GMimeMultipart *mpayload;
> +const char *protected_header_parameter;
> +GMimeTextPart *legacy_display;
> +char *legacy_display_header_text = NULL;
> +GMimeStream *stream = NULL;
> +GMimeParser *parser = NULL;
> +GMimeObject *legacy_header_object = NULL, *first;
> +GMimeHeaderList *legacy_display_headers = NULL, *protected_headers =
> NULL;
> +bool ret = false;
> +
> +if (! g_mime_content_type_is_type (g_mime_object_get_content_type
> (payload),
> +"multipart", "mixed"))
> + return false;
> +protected_header_parameter = g_mime_object_get_content_type_parameter
> (payload, "protected-headers");
> +if ((! protected_header_parameter) || strcmp
> (protected_header_parameter, "v1"))
> + return false;
> +if (! GMIME_IS_MULTIPART (payload))
> + return false;
> +mpayload = GMIME_MULTIPART (payload);
> +if (mpayload == NULL)
> + return false;
> +if (g_mime_multipart_get_count (mpayload) != 2)
> + return false;
> +first = g_mime_multipart_get_part (mpayload, 0);
> +if (! g_mime_content_type_is_type (g_mime_object_get_content_type
> (first),
> +"text", "rfc822-headers"))
> + return false;
> +protected_header_parameter = g_mime_object_get_content_type_parameter
> (first, "protected-headers");
> +if ((! protected_header_parameter) || strcmp
> (protected_header_parameter, "v1"))
> + return false;
> +if (! GMIME_IS_TEXT_PART (first))
> + return false;
> +
> +/* ensure that the headers in the first part all match the values
> + * found in the payload's own protected headers! if they don't,
> + * we should not treat this as a valid "legacy-display" part.
> + *
> + * Crafting a GMimeHeaderList object from the content of the
> + * text/rfc822-headers part is pretty clumsy; we should probably
> + * push something into GMime that makes this a one-shot
> + * operation. */
> +if ((protected_headers = g_mime_object_get_header_list (payload),
> protected_headers) &&
> + (legacy_display = GMIME_TEXT_PART (first), legacy_display) &&
> + (legacy_display_header_text = g_mime_text_part_get_text
> (legacy_display), legacy_display_header_text) &&
> + (stream = g_mime_stream_mem_new_with_buffer
> (legacy_display_header_text, strlen (legacy_display_header_text)), stream) &&
> + (g_mime_stream_write (stream, "\r\n\r\n", 4) == 4) &&
> + (g_mime_stream_seek (stream, 0, GMIME_STREAM_SEEK_SET) == 0) &&
> + (parser = g_mime_parser_new_with_stream (stream), parser) &&
> + (legacy_header_object = g_mime_parser_construct_part (parser, NULL),
> legacy_header_object) &&
> + (legacy_display_headers = g_mime_object_get_header_list
> (legacy_header_object), legacy_display_headers)) {
> + /* walk through legacy_display_headers, comparing them against
This may be a noob question, but why the comma operators after the
assignment expressions? Wouldn't they evaluate to the same thing?
> + * their values in the protected_headers: */
> + ret = true;
> + for (int i = 0; i < g_mime_header_list_get_count
> (legacy_display_headers); i++) {
> + GMimeHeader *dh = g_mime_header_list_get_header_at
> (legacy_display_headers, i);
> + if (dh == NULL) {
> + ret = false;
> + break;
> + }
> + GMimeHeader *ph = g_mime_header_list_get_header (protected_headers,
> g_mime_header_get_name (dh));
> + if (ph == NULL) {
> + ret = false;
> + break;
> + }
> + if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value
> (ph))) {
> + ret = false;
> + break;
> + }
> + }
> +}
> +
> +if (legacy_display_header_text)
> + g_free (legacy_display_header_text);
> +if (stream)
> + g_object_unref (stream);
> +if (parser)
> + g_object_unref (parser);
> +if (legacy_header_object)
> +
[PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display
This is a utility function designed to make it easier to
"fast-forward" past a legacy-display part associated with a
cryptographic envelope, and show the user the intended message body.
The bulk of the ugliness in here is in the test function
_notmuch_crypto_payload_has_legacy_display, which tests all of the
things we'd expect to be true in a a cryptographic payload that
contains a legacy display part.
Signed-off-by: Daniel Kahn Gillmor
---
util/repair.c | 98 +++
util/repair.h | 17 +
2 files changed, 115 insertions(+)
diff --git a/util/repair.c b/util/repair.c
index f91c1244..0809f7b4 100644
--- a/util/repair.c
+++ b/util/repair.c
@@ -18,4 +18,102 @@
* Authors: Daniel Kahn Gillmor
*/
+#include
#include "repair.h"
+
+
+static bool
+_notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
+{
+GMimeMultipart *mpayload;
+const char *protected_header_parameter;
+GMimeTextPart *legacy_display;
+char *legacy_display_header_text = NULL;
+GMimeStream *stream = NULL;
+GMimeParser *parser = NULL;
+GMimeObject *legacy_header_object = NULL, *first;
+GMimeHeaderList *legacy_display_headers = NULL, *protected_headers = NULL;
+bool ret = false;
+
+if (! g_mime_content_type_is_type (g_mime_object_get_content_type
(payload),
+ "multipart", "mixed"))
+ return false;
+protected_header_parameter = g_mime_object_get_content_type_parameter
(payload, "protected-headers");
+if ((! protected_header_parameter) || strcmp (protected_header_parameter,
"v1"))
+ return false;
+if (! GMIME_IS_MULTIPART (payload))
+ return false;
+mpayload = GMIME_MULTIPART (payload);
+if (mpayload == NULL)
+ return false;
+if (g_mime_multipart_get_count (mpayload) != 2)
+ return false;
+first = g_mime_multipart_get_part (mpayload, 0);
+if (! g_mime_content_type_is_type (g_mime_object_get_content_type (first),
+ "text", "rfc822-headers"))
+ return false;
+protected_header_parameter = g_mime_object_get_content_type_parameter
(first, "protected-headers");
+if ((! protected_header_parameter) || strcmp (protected_header_parameter,
"v1"))
+ return false;
+if (! GMIME_IS_TEXT_PART (first))
+ return false;
+
+/* ensure that the headers in the first part all match the values
+ * found in the payload's own protected headers! if they don't,
+ * we should not treat this as a valid "legacy-display" part.
+ *
+ * Crafting a GMimeHeaderList object from the content of the
+ * text/rfc822-headers part is pretty clumsy; we should probably
+ * push something into GMime that makes this a one-shot
+ * operation. */
+if ((protected_headers = g_mime_object_get_header_list (payload),
protected_headers) &&
+ (legacy_display = GMIME_TEXT_PART (first), legacy_display) &&
+ (legacy_display_header_text = g_mime_text_part_get_text
(legacy_display), legacy_display_header_text) &&
+ (stream = g_mime_stream_mem_new_with_buffer
(legacy_display_header_text, strlen (legacy_display_header_text)), stream) &&
+ (g_mime_stream_write (stream, "\r\n\r\n", 4) == 4) &&
+ (g_mime_stream_seek (stream, 0, GMIME_STREAM_SEEK_SET) == 0) &&
+ (parser = g_mime_parser_new_with_stream (stream), parser) &&
+ (legacy_header_object = g_mime_parser_construct_part (parser, NULL),
legacy_header_object) &&
+ (legacy_display_headers = g_mime_object_get_header_list
(legacy_header_object), legacy_display_headers)) {
+ /* walk through legacy_display_headers, comparing them against
+* their values in the protected_headers: */
+ ret = true;
+ for (int i = 0; i < g_mime_header_list_get_count
(legacy_display_headers); i++) {
+ GMimeHeader *dh = g_mime_header_list_get_header_at
(legacy_display_headers, i);
+ if (dh == NULL) {
+ ret = false;
+ break;
+ }
+ GMimeHeader *ph = g_mime_header_list_get_header (protected_headers,
g_mime_header_get_name (dh));
+ if (ph == NULL) {
+ ret = false;
+ break;
+ }
+ if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value
(ph))) {
+ ret = false;
+ break;
+ }
+ }
+}
+
+if (legacy_display_header_text)
+ g_free (legacy_display_header_text);
+if (stream)
+ g_object_unref (stream);
+if (parser)
+ g_object_unref (parser);
+if (legacy_header_object)
+ g_object_unref (legacy_header_object);
+
+return ret;
+}
+
+GMimeObject *
+_notmuch_repair_crypto_payload_skip_legacy_display (GMimeObject *payload)
+{
+if (_notmuch_crypto_payload_has_legacy_display (payload)) {
+ return g_mime_multipart_get_part (GMIME_MULTIPART (payload), 1);
+} else {
+ return
