[PATCH] Do not attept to output part raw if part is not GMimePart.

[PATCH] Do not attept to output part raw if part is not GMimePart.

[Jameson Graef Rollins]

This was a minor oversite in checking of part type when outputing
content raw.  This was causing gmime was to throw an exception to
stderr.

Unfortunately the gmime exception was not being caught by notmuch, or
the test suite.  I'm not sure if notmuch should have done anything in
this case, but certainly the test suite should be capable of detecting
that something unexpected was output to stderr.
---
 notmuch-show.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 9267d02..3b4f775 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -646,14 +646,15 @@ format_part_content_raw (GMimeObject *part)
 {
     GMimeStream *stream_stdout;
     GMimeStream *stream_filter = NULL;
-    GMimeDataWrapper *wrapper;
+    GMimeDataWrapper *wrapper = NULL;
 
     stream_stdout = g_mime_stream_file_new (stdout);
     g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
 
     stream_filter = g_mime_stream_filter_new (stream_stdout);
 
-    wrapper = g_mime_part_get_content_object (GMIME_PART (part));
+    if (GMIME_IS_PART (part))
+	wrapper = g_mime_part_get_content_object (GMIME_PART (part));
 
     if (wrapper && stream_filter)
 	g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
-- 
1.7.4.4

[PATCH] Do not attept to output part raw if part is not GMimePart.

[Jameson Graef Rollins]

This was a minor oversite in checking of part type when outputing
content raw.  This was causing gmime was to throw an exception to
stderr.

Unfortunately the gmime exception was not being caught by notmuch, or
the test suite.  I'm not sure if notmuch should have done anything in
this case, but certainly the test suite should be capable of detecting
that something unexpected was output to stderr.
---
This is a cleaner version of this patch.

 notmuch-show.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 9267d02..59f7078 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -644,6 +644,9 @@ format_part_end_json (GMimeObject *part)
 static void
 format_part_content_raw (GMimeObject *part)
 {
+    if (! GMIME_IS_PART (part))
+	return;
+
     GMimeStream *stream_stdout;
     GMimeStream *stream_filter = NULL;
     GMimeDataWrapper *wrapper;
-- 
1.7.4.4

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Carl Worth]

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

On Fri,  3 Jun 2011 10:01:06 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> This was a minor oversite in checking of part type when outputing
> content raw.  This was causing gmime was to throw an exception to
> stderr.
> 
> Unfortunately the gmime exception was not being caught by notmuch, or
> the test suite.  I'm not sure if notmuch should have done anything in
> this case, but certainly the test suite should be capable of detecting
> that something unexpected was output to stderr.

I'd like to investigate this case a little bit to help answer the
question of whether "notmuch should have done anything in this case".

But if I apply the patch then I won't be able to. ;-)

Can you give me instructions for how to replicate this bug before I
apply the patch?

-Carl

-- 
carl.d.worth@intel.com

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

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Jameson Graef Rollins]

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

On Thu, 23 Jun 2011 16:33:18 -0700, Carl Worth <cworth@cworth.org> wrote:
> I'd like to investigate this case a little bit to help answer the
> question of whether "notmuch should have done anything in this case".

Hi, Carl.  You can see this error if you try to output raw a multipart/*
or message/rfc822 part, ie:

servo:~ 0$ notmuch show --part=1 --format=raw id:"87wrgccedd.fsf@yoom.home.cworth.org" >/dev/null

(process:29838): GLib-GObject-WARNING **: invalid cast from `GMimeMultipartSigned' to `GMimePart'

(process:29838): gmime-CRITICAL **: g_mime_part_get_content_object: assertion `GMIME_IS_PART (mime_part)' failed
servo:~ 0$ 

Gmime seems to be successfully casting the part into GMimePart and then
outputting the content, but it does produce the warning on stderr.

jamie.

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

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Austin Clements]

On Mon, Jun 27, 2011 at 1:07 PM, Jameson Graef Rollins
<jrollins@finestructure.net> wrote:
> On Thu, 23 Jun 2011 16:33:18 -0700, Carl Worth <cworth@cworth.org> wrote:
>> I'd like to investigate this case a little bit to help answer the
>> question of whether "notmuch should have done anything in this case".
>
> Hi, Carl.  You can see this error if you try to output raw a multipart/*
> or message/rfc822 part, ie:
>
> servo:~ 0$ notmuch show --part=1 --format=raw id:"87wrgccedd.fsf@yoom.home.cworth.org" >/dev/null
>
> (process:29838): GLib-GObject-WARNING **: invalid cast from `GMimeMultipartSigned' to `GMimePart'
>
> (process:29838): gmime-CRITICAL **: g_mime_part_get_content_object: assertion `GMIME_IS_PART (mime_part)' failed
> servo:~ 0$
>
> Gmime seems to be successfully casting the part into GMimePart and then
> outputting the content, but it does produce the warning on stderr.

Just to clarify my understanding, --format=raw is only intended to
work on either the whole message (special-cased in do_show_single) or
a leaf MIME part, and in any other case, it will output nothing?  The
raw output test cases seem pretty thin.

(BTW, I believe the show restructuring I have under way should both
lift this restriction and eliminate the special case.)

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Daniel Kahn Gillmor]

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

On 06/27/2011 04:43 PM, Austin Clements wrote:
> Just to clarify my understanding, --format=raw is only intended to
> work on either the whole message (special-cased in do_show_single) or
> a leaf MIME part, and in any other case, it will output nothing?  The
> raw output test cases seem pretty thin.

I personally think that --format=raw *should* work on non-leaf parts; i
don't know if that was the original intent.

> (BTW, I believe the show restructuring I have under way should both
> lift this restriction and eliminate the special case.)

I hope the tests for this feature can eventually include some weird
compound parts with whitespace-wrapped header lines, odd MIME
delimiters, character encodings, etc.  It would be really good if the
--part=whatever --format=raw output produces byte-for-byte identical
streams.  Both PGP/MIME and S/MIME signatures rely on explicit opacity
of the signed MIME parts, so being able to retrieve the signed data
precisely would be quite useful for weird corner cases.

	--dkg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 1030 bytes --]

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Jameson Graef Rollins]

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

On Mon, 27 Jun 2011 16:43:36 -0400, Austin Clements <amdragon@mit.edu> wrote:
> Just to clarify my understanding, --format=raw is only intended to
> work on either the whole message (special-cased in do_show_single) or
> a leaf MIME part, and in any other case, it will output nothing?  The
> raw output test cases seem pretty thin.

Hey, Austin.  The raw part output works for *any* part, be it leaf part,
multipart, message/rfc822, etc.  I added a bunch of tests for raw part
output that should cover all of this, although I don't think they've
been pulled into master yet.

I think there are a lot of open questions about what should be output
for multipart raw.  We should output _something_, though.  I think we
can fix all of this up for 0.7, based on the work you've already done,
after 0.6 is released.

jamie.

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

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Austin Clements]

Quoth Jameson Graef Rollins on Jun 27 at  2:44 pm:
> On Mon, 27 Jun 2011 16:43:36 -0400, Austin Clements <amdragon@mit.edu> wrote:
> > Just to clarify my understanding, --format=raw is only intended to
> > work on either the whole message (special-cased in do_show_single) or
> > a leaf MIME part, and in any other case, it will output nothing?  The
> > raw output test cases seem pretty thin.
> 
> Hey, Austin.  The raw part output works for *any* part, be it leaf part,
> multipart, message/rfc822, etc.  I added a bunch of tests for raw part
> output that should cover all of this, although I don't think they've
> been pulled into master yet.

Oh, right, of course.  show_message_part will walk into the parts, so
format_part_content_raw will still be called on the leafs of a
requested multipart.  Though, this approach results in each leaf being
transfer decoded and printed individually, so if you ask for a
multipart, you won't get the "raw" contents of the multipart (unless
it's part 0), so much as you get the concatenated "raw" contents of
each part in the multipart.

Daniel, is this the problem that you're getting at with "opacity"?
That if you ask for a multipart, you should effectively get a slice
out of the original message bytes (since multipart/* parts can't have
non-identity transfer encodings).  Are you also saying that should
extend to transfer encoded leaf parts, too?

> I think there are a lot of open questions about what should be output
> for multipart raw.  We should output _something_, though.  I think we
> can fix all of this up for 0.7, based on the work you've already done,
> after 0.6 is released.

Yes, hopefully.  That's why I'm making sure I understand the issues
here.  ]:--8)

interaction between --format=raw and multipart handling [was: Re: Do not attept to output part raw if part is not GMimePart.]

[Daniel Kahn Gillmor]

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

On 06/27/2011 06:07 PM, Austin Clements wrote:
> Oh, right, of course.  show_message_part will walk into the parts, so
> format_part_content_raw will still be called on the leafs of a
> requested multipart.  Though, this approach results in each leaf being
> transfer decoded and printed individually, so if you ask for a
> multipart, you won't get the "raw" contents of the multipart (unless
> it's part 0), so much as you get the concatenated "raw" contents of
> each part in the multipart.


let's take two labeled examples:

A└┬╴multipart/signed 58292 bytes
B ├┬╴multipart/mixed 56553 bytes
C │├╴text/plain 1278 bytes
D │├╴text/plain attachment [grub-install.out] 54109 bytes
E │└╴text/x-diff attachment [597538.patch] 496 bytes
F └╴application/pgp-signature attachment [signature.asc] 900 bytes


X└┬╴multipart/signed 3863 bytes
Y ├╴text/plain 1857 bytes
Z └╴application/pgp-signature attachment [signature.asc] 900 bytes

(i know, you won't use "A" or "Z" as part IDs once we have hierarchical
part numbers, but consider them placeholders).

if parts F or Z are ever going to be useful (e.g. to some external
process that wants to validate the signature by hand), then the tool
needs to provide some way of producing parts B and Y in a pristine form
(that is, including MIME headers and without interpreting/applying any
transfer encodings).

Perhaps this means there are two flavors of "raw" that we should be
distinguishing, something like:

 0) "source" -- the equivalent to viewing the source of the message,
with headers and without attempting to reverse transfer-encodings, etc.

 1) "rare" -- (not entirely raw, but still bloody, ha ha) strip headers,
reverse transfer encodings, etc.

I think our current implementation of --format=raw emits "source" when
applied to the entire message, but "rare" when applied to one of the parts.

I'm suggesting that it might be useful to be able to get "source" of a
part.  (and perhaps it might also be useful to get the whole message
"rare" sometimes?)

My first instinct was: if it's multipart, provide "source", if it's
single-part, provide "rare".  But that fails for the XYZ case above --
we'd need Y (which is single-part) to be provided as "source" if we were
ever to be able to make use of Z on its own, so i don't think it'll be
that simple.

OTOH, i'm not sure that "rare" is particularly meaningful for non-leaf
parts.

> Daniel, is this the problem that you're getting at with "opacity"?

the origin of the term "opaque" used in this context can be found in the
definition of multipart/signed:

 https://tools.ietf.org/html/rfc1847#section-2.1

> That if you ask for a multipart, you should effectively get a slice
> out of the original message bytes (since multipart/* parts can't have
> non-identity transfer encodings).  Are you also saying that should
> extend to transfer encoded leaf parts, too?

hmm.  is it true that multipart/* parts can't have non-identity transfer
encodings?  that would simplify some things, but i don't have a
reference handy that says it's the case.

At any rate, i'm not sure it affects the need for being able to emit
both "rare" and "source" forms of at least the leaf (non-multipart) parts.

i hope this is all at least somewhat clarifying and not just adding to
the confusion,

	--dkg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 1030 bytes --]

Re: interaction between --format=raw and multipart handling [was: Re: Do not attept to output part raw if part is not GMimePart.]

[Austin Clements]

On Mon, Jun 27, 2011 at 6:41 PM, Daniel Kahn Gillmor
<dkg@fifthhorseman.net> wrote:
> On 06/27/2011 06:07 PM, Austin Clements wrote:
>> Oh, right, of course.  show_message_part will walk into the parts, so
>> format_part_content_raw will still be called on the leafs of a
>> requested multipart.  Though, this approach results in each leaf being
>> transfer decoded and printed individually, so if you ask for a
>> multipart, you won't get the "raw" contents of the multipart (unless
>> it's part 0), so much as you get the concatenated "raw" contents of
>> each part in the multipart.
>
>
> let's take two labeled examples:
>
> A└┬╴multipart/signed 58292 bytes
> B ├┬╴multipart/mixed 56553 bytes
> C │├╴text/plain 1278 bytes
> D │├╴text/plain attachment [grub-install.out] 54109 bytes
> E │└╴text/x-diff attachment [597538.patch] 496 bytes
> F └╴application/pgp-signature attachment [signature.asc] 900 bytes
>
>
> X└┬╴multipart/signed 3863 bytes
> Y ├╴text/plain 1857 bytes
> Z └╴application/pgp-signature attachment [signature.asc] 900 bytes
>
> (i know, you won't use "A" or "Z" as part IDs once we have hierarchical
> part numbers, but consider them placeholders).
>
> if parts F or Z are ever going to be useful (e.g. to some external
> process that wants to validate the signature by hand), then the tool
> needs to provide some way of producing parts B and Y in a pristine form
> (that is, including MIME headers and without interpreting/applying any
> transfer encodings).
>
> Perhaps this means there are two flavors of "raw" that we should be
> distinguishing, something like:
>
>  0) "source" -- the equivalent to viewing the source of the message,
> with headers and without attempting to reverse transfer-encodings, etc.
>
>  1) "rare" -- (not entirely raw, but still bloody, ha ha) strip headers,
> reverse transfer encodings, etc.
>
> I think our current implementation of --format=raw emits "source" when
> applied to the entire message, but "rare" when applied to one of the parts.

Yes.

> I'm suggesting that it might be useful to be able to get "source" of a
> part.  (and perhaps it might also be useful to get the whole message
> "rare" sometimes?)
>
> My first instinct was: if it's multipart, provide "source", if it's
> single-part, provide "rare".  But that fails for the XYZ case above --
> we'd need Y (which is single-part) to be provided as "source" if we were
> ever to be able to make use of Z on its own, so i don't think it'll be
> that simple.
>
> OTOH, i'm not sure that "rare" is particularly meaningful for non-leaf
> parts.
>
>> That if you ask for a multipart, you should effectively get a slice
>> out of the original message bytes (since multipart/* parts can't have
>> non-identity transfer encodings).  Are you also saying that should
>> extend to transfer encoded leaf parts, too?
>
> hmm.  is it true that multipart/* parts can't have non-identity transfer
> encodings?  that would simplify some things, but i don't have a
> reference handy that says it's the case.

RFC 2045, section 6.4: "If an entity is of type "multipart" the
Content-Transfer-Encoding is not permitted to have any value other
than "7bit", "8bit" or "binary"."  (And, for completeness, section
6.2: "The Content-Transfer-Encoding values "7bit", "8bit", and
"binary" all mean that the identity (i.e. NO) encoding transformation
has been  performed.")

> At any rate, i'm not sure it affects the need for being able to emit
> both "rare" and "source" forms of at least the leaf (non-multipart) parts.
>
> i hope this is all at least somewhat clarifying and not just adding to
> the confusion,

Thanks.  That's actually very informative and solidifies some of
what's been slowly coagulating in my mind.

I was also thinking about the two output variants you describe
(though, being less clever, I was thinking "raw" and "decoded").  The
fact that multipart/* parts can only have identity encodings makes me
wonder if the two could be merged by thinking of the decoded content
of a leaf part as a child/body to the original, encoded part.  On the
other hand, that doesn't make sense for other formats, so perhaps
that's not a fruitful approach.

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Jameson Graef Rollins]

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

On Fri,  3 Jun 2011 10:01:06 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> This was a minor oversite in checking of part type when outputing
> content raw.  This was causing gmime was to throw an exception to
> stderr.

I'm finding it perplexing that this patch still hasn't been applied,
considering how small it is and that it fixes a real bug.  Isn't anyone
else experiencing this same issue?  I would really like to hear either
way, if people either are or are *not* experiencing this issue.

To reiterate, this bug can be triggered very simply with the following
command:

$ notmuch show --part=1 --format=raw id:"87wrgccedd.fsf@yoom.home.cworth.org" >/dev/null

Please do let me know want you find.

jamie.

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

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Dmitry Kurochkin]

Hi Jamie.

On Tue, 06 Sep 2011 09:43:00 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Fri,  3 Jun 2011 10:01:06 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > This was a minor oversite in checking of part type when outputing
> > content raw.  This was causing gmime was to throw an exception to
> > stderr.
> 
> I'm finding it perplexing that this patch still hasn't been applied,
> considering how small it is and that it fixes a real bug.  Isn't anyone
> else experiencing this same issue?  I would really like to hear either
> way, if people either are or are *not* experiencing this issue.
> 
> To reiterate, this bug can be triggered very simply with the following
> command:
> 
> $ notmuch show --part=1 --format=raw id:"87wrgccedd.fsf@yoom.home.cworth.org" >/dev/null
> 
> Please do let me know want you find.
> 

For me this results in:

(process:15727): GLib-GObject-WARNING **: invalid cast from `GMimeMultipart' to `GMimePart'

(process:15727): gmime-CRITICAL **: g_mime_part_get_content_object: assertion `GMIME_IS_PART (mime_part)' failed

(process:15727): GLib-GObject-WARNING **: invalid cast from `GMimeMultipartSigned' to `GMimePart'

(process:15727): gmime-CRITICAL **: g_mime_part_get_content_object: assertion `GMIME_IS_PART (mime_part)' failed

HTH,
  Dmitry

> jamie.
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Jameson Graef Rollins]

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

Everyone that I know of who has tried to reproduce this bug has been
able to.  I vote that we include this patch in 0.8, so that we can get
it out of the way.

jamie.

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

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[David Bremner]

On Tue, 06 Sep 2011 15:51:42 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:

> Everyone that I know of who has tried to reproduce this bug has been
> able to.  I vote that we include this patch in 0.8, so that we can get
> it out of the way.

I have pushed this to master. Sorry, I still don't plan to include it in
0.8.  I think it is better to have a short strict freeze, than a long
friendly one.

d

Re: [PATCH] Do not attept to output part raw if part is not GMimePart.

[Jameson Graef Rollins]

On Wed, 07 Sep 2011 18:46:10 -0300, David Bremner <david@tethera.net> wrote:
> I have pushed this to master. Sorry, I still don't plan to include it in
> 0.8.  I think it is better to have a short strict freeze, than a long
> friendly one.

Ok, thanks.  Just as long as I don't have to push to get this fix
applied anymore.

This will be more an issue for the users of the 0.8 branch, anyway,
since I run my own version where this patch was applied many moons ago.

jamie.