Created attachment 147958 [details] [review]
my take on the malloc() stuf in tnef-plugin.c

0) That does indeed look like a leak.

1) There seems to be another leak caused by this (around line 350):
    filename = (variableLength*)malloc(sizeof(variableLength));

2) I found the code of this plugin rather malloc() happy. Attached is a patch (currently against 2.28.0) that I've been using for some releases. (I sent a similar patch (against 2.23.4), about one and a half year ago, to some developers but never received any reaction. Maybe they were to polite to tell me what was wrong with my patch ...)

(In reply to comment #1)
> Created an attachment (id=147958) [details] [review]

I used the excellent cppcheck tool (hosted on sourceforge)
to find these problems in the tnef plugin

[./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:108]: (error) Memory leak: tnef
[./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:283]: (error) Memory leak: buf
[./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:692]: (error) Resource leak: fptr
[./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:1050]: (error) Memory leak: buf

Some, all or none might be worth fixing.

(In reply to comment #2)
> I used the excellent cppcheck tool (hosted on sourceforge)
> to find these problems in the tnef plugin
> 
> [./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:108]: (error) Memory
> leak: tnef
> [./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:283]: (error) Memory
> leak: buf
> [./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:692]: (error)
> Resource leak: fptr

What is a resource leak? failure to close an opened file?

> [./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:1050]: (error)
> Memory leak: buf
> 
> Some, all or none might be worth fixing.

That last statement might be true or false. Also, cppcheck may or may not miss all leaks.

For what it's worth: it seems my patch addresses all three memory leaks you copy/pasted above. The patch doesn't address files kept open.

Suggestions for improvements of my patch are appreciated.

(In reply to comment #3)
> > > [./evolution-2.28.0/plugins/tnef-attachments/tnef-plugin.c:692]: (error)
> > Resource leak: fptr
> 
> What is a resource leak? failure to close an opened file?

It turns out fptr is never closed() in save VCard(). So, that does indeed seem to be what a resource leak is.

(In reply to comment #3)
> What is a resource leak? failure to close an opened file?

A leak that isn't a memory leak is a resource leak, usually leaking
on a file, sometimes a socket or a pipe or something else the 
operating system only has a few of per process.

Usually they don't matter for short lived programs because
the operating systems does process cleanup on exit, but can kill
long lived programs which don't call exit.

(In reply to comment #5)
> Usually they don't matter for short lived programs because
> the operating systems does process cleanup on exit, but can kill
> long lived programs which don't call exit.

Can anybody say whether it differs that this resource leak happens here in a plugin (ie, does evolution somehow calls something equivalent to exit() on the plugin code)? I really wouldn't know.

Created attachment 147965 [details] [review]
filepath is global (to this plugin)

0) While we're on the subject of real defects in tnef-plugin.c: filepath is global (to this plugin). Not good. Could lead to race conditions.

1) I remember that this race condition could be triggered by quickly alternating between different messages with a TNEF attachment. Had some strange effect on the display of files enclosed in that TNEF attachment.

2) Patch attached (also sent to developers quite some time ago).

Just a private note, that might also trigger someone else to look at this: not everything created with a *_new() call is camel_object_unref'ed (ie, "mp" and "part"). I'm not (yet) sure whether that should actually always be done. I hope to have a look at that, sometime ...

The first patch looks good, on the first look. But the second not as that, because:
a) evolution is not using g_assert in newly written code, there had been done
   a decision to not doing so more than a year ago

b) the glib types are preferred in evolution code, please use gchar instead
   of char

c) if your functions are not going to modify the file name, use
   const gchar * as a parameter, to indicate so.

d) are you able to attach here a test message, so I will be able to check
   patches in action, please? I do not have any tnef message :(

(In reply to comment #9)
> d) are you able to attach here a test message, so I will be able to check
>    patches in action, please? I do not have any tnef message :(

Only messages sent by relatives. Would be impolite to attach, I'm afraid.

(In reply to comment #10)
> Only messages sent by relatives. Would be impolite to attach, I'm afraid.

Yes, I agree. Maybe a little test message sent by them, then stripped private information like server addresses and such from there?

0) Found two in bugzilla:
bug #331932 (message text enclosed in rtf; set saveRtf to '1' in code)
bug #483558 (enclosed icalender attachment; maybe you need to change ".vcf" to ".ics" in the code to actually see it)

The headers copy/pasted in the reports need to "massaged" to be valid and a "From " header must be prepended.

1) google for:

    filename="winmail.dat" "This is a multi-part message in MIME format."

That will give thousands of hits, some of which can be (manually) made into a proper mbox (one often needs to add a "MIME-Version: 1.0" header and a 
"Content-Type: multipart/mixed; boundary="[...]"" header). The few messages I tried only had the message text enclosed in rtf. Would be nice to find things like actual attachments or forwarded or replied messages.

Here's a link to a message with an attachment (DBG_LOG.TGZ) and an enclosed copy of the text of the message in rtf: http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;mbox=yes;bug=107532 (a message sent to a debian bug reporting system).

Thanks for all the pointers. The later of comment #12 is very nice, it crashes Evolution for me, with this information:
> *** buffer overflow detected ***: evolution terminated
> ======= Backtrace: =========
> /lib/libc.so.6(__fortify_fail+0x48)[0x11b9bd8]
> /lib/libc.so.6[0x11b7d90]
> /lib/libc.so.6(__strcpy_chk+0x44)[0x11b7074]
> /usr/lib/libytnef.so.0(DecompressRTF+0x48)[0x9962ef8]
> .../liborg-gnome-tnef-attachments.so(saveVCalendar+0x76f)[0x1468823]

most likely due to my old F11 libytnef library.

Paul, by the way, I wasn't able to compile those patches, it claims this:
> tnef-plugin.c: In function ‘processTnef’:
> tnef-plugin.c:220: warning: ISO C90 forbids mixed declarations and code
> tnef-plugin.c:310: error: invalid operands to binary & (have ‘gint’ and
>   ‘TNEFStruct’)
> make[1]: *** [liborg_gnome_tnef_attachments_la-tnef-plugin.lo] Error 1
> make: *** [all] Error 2

were you able to compile it?

(In reply to comment #14)
> Thanks for all the pointers. The later of comment #12 is very nice, it crashes
> Evolution for me, with this information:
> > *** buffer overflow detected ***: evolution terminated
> > ======= Backtrace: =========
> > /lib/libc.so.6(__fortify_fail+0x48)[0x11b9bd8]
> > /lib/libc.so.6[0x11b7d90]
> > /lib/libc.so.6(__strcpy_chk+0x44)[0x11b7074]
> > /usr/lib/libytnef.so.0(DecompressRTF+0x48)[0x9962ef8]
> > .../liborg-gnome-tnef-attachments.so(saveVCalendar+0x76f)[0x1468823]
> 
> most likely due to my old F11 libytnef library.

Can you repeat with debugging symbols?
 
> Paul, by the way, I wasn't able to compile those patches, it claims this:
> > tnef-plugin.c: In function ‘processTnef’:
> > tnef-plugin.c:220: warning: ISO C90 forbids mixed declarations and code
> > tnef-plugin.c:310: error: invalid operands to binary & (have ‘gint’ and
> >   ‘TNEFStruct’)
> > make[1]: *** [liborg_gnome_tnef_attachments_la-tnef-plugin.lo] Error 1
> > make: *** [all] Error 2
> 
> were you able to compile it?

The patches were against F12 evolution 2.28 (I still need to upgrade to 2.29 for F13). Shall I forward port my patches (against 2.29 and/or master)?

+ Trace 219204

I do not think it's evolution's issue, it's in libytnef code. And it does that even without your patch, which I see wasn't clear from my previous comment.

But the error on compilation is correct, and that warning as well. But the patches are otherwise cleanly applicable to actual master. Look the error line, you did a type there, I've no idea why it didn't claim to you. Maybe a different compiler? (the warning is there since changes in master, but error shouldn't be.)

OK, I tested the patch (its slightly modified version to be able to compile it, to not have gcc claim anything on it, without changes said in comment #9. The result is quite positive, some leaks had been fixed based on valgrind log, but some still left (I didn't investigate it further, maybe it's just my libytnef version, I do not know):
==26810== Invalid read of size 8
==26810==    at 0x1BD41588: TNEFRendData (string3.h:52)
==26810==    by 0x1EDE371F: ???
==26810==    by 0x30A82AFFFF: (within /lib64/libglib-2.0.so.0.2000.4)
==26810==  Address 0x1f86e400 is 8 bytes inside a block of size 14 alloc'd
==26810==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==26810==    by 0x1BD3FE5B: TNEFParse (ytnef.c:997)
==26810==    by 0x1BD402FB: TNEFParseFile (ytnef.c:890)
==26810==    by 0x1BB372A3: org_gnome_format_tnef (tnef-plugin.c:112)
==26810==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==26810==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==26810==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==26810==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==26810==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==26810==    by 0xEC1F976: em_format_part (em-format.c:705)
==26810==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==26810==    by 0xEC1F87F: em_format_part_as (em-format.c:676)

==26810== 0 bytes in 1 blocks are definitely lost in loss record 1 of 297
==26810==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==26810==    by 0x1BD41DAE: TNEFFillMapi (ytnef.c:419)
==26810==    by 0x1BD420B7: TNEFAttachmentMAPI (ytnef.c:316)
==26810==    by 0x1BD3FEEE: TNEFParse (ytnef.c:1026)
==26810==    by 0x1BD402FB: TNEFParseFile (ytnef.c:890)
==26810==    by 0x1BB372A3: org_gnome_format_tnef (tnef-plugin.c:112)
==26810==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==26810==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==26810==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==26810==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==26810==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==26810==    by 0xEC1F976: em_format_part (em-format.c:705)

0) I haven't yet found the offending lines in the version I use over here. What is the content of the error line and the warning line?

1) I'm not familiar with the log type of comment 17, but as far as I can see all issues are inside libytnef. I seem to remember libytnef compiling noisily (mostly signed/unsigned mismatches if I recall correctly).

(In reply to comment #18)
> 0) I haven't yet found the offending lines in the version I use over here. What
> is the content of the error line and the warning line?

Ah, found it

    filedata->size-16, i&emb_tnef) != -1) {

That's a nice EVIM (masked by a later patch, I'm sure, in my stack of patches)!

(In reply to comment #16)
> I do not think it's evolution's issue, it's in libytnef code. And it does that
> even without your patch, which I see wasn't clear from my previous comment.

libytnef crashed here (in line 1331):
1328 
1329     comp_Prebuf.size = strlen(RTF_PREBUF);
1330     comp_Prebuf.data = calloc(comp_Prebuf.size, 1);
1331     strcpy(comp_Prebuf.data, RTF_PREBUF);
1332 
1333     src = p->data;
1334     in = 0;

This is entirely untested, but maybe that should be:
[...]
comp_Prebuf.size = strlen(RTF_PREBUF) + 1;
comp_Prebuf.data = calloc(comp_Prebuf.size, 1);
[...]

If you I agree I could try to write a trivial patch (for Fedora, upstream is dormant).

(In reply to comment #20)
> This is entirely untested, but maybe that should be:
> [...]
> comp_Prebuf.size = strlen(RTF_PREBUF) + 1;
> comp_Prebuf.data = calloc(comp_Prebuf.size, 1);
> [...]

By the way, as far as I can see none of the returns of the 23 calls of [cm]alloc() are checked. I'm not sure what might be an elegant way to catch NULL returns in ytnef.c

We moved slightly out of the initial subject here. I'm happy to know what to change in libytnef, and I'll do that here locally, to test any followup patch. Thanks for pointing me to the right place.

So we are waiting for a patch for evolution, one patch, which will cover discovered issues on evolution side.

(In reply to comment #20)
> This is entirely untested, but maybe that should be:
> [...]
> comp_Prebuf.size = strlen(RTF_PREBUF) + 1;
> comp_Prebuf.data = calloc(comp_Prebuf.size, 1);
> [...]
> 
> If you I agree I could try to write a trivial patch (for Fedora, upstream is
> dormant).

The change seems correct, but please try to push it through upstream, and maybe contact distro maintainers to include fix until the upstream does that itself. I believe it's just that fedora is more strict, but the failure (write out of the buffer) is an issue on all distros.

(In reply to comment #21)
> By the way, as far as I can see none of the returns of the 23 calls of
> [cm]alloc() are checked. I'm not sure what might be an elegant way to catch
> NULL returns in ytnef.c

If they are using glib in there, then might worth it use a g_malloc/g_calloc wrappers, which aborts on failure.

Created attachment 148348 [details] [review]
compilable; also plug resource leak

0) This version should actually compile.

1) It also plugs the resource leak a tool called cppcheck apparently discovered (see comment #2).

Created attachment 148350 [details] [review]
filepath is global (to this plugin) v2

Addresses all issues raised in the review in comment #9.

Created attachment 148351 [details] [review]
[RFC] plug leaking of objects

0) See comment #8.

1) Tested lightly. No apparent problems (ie, evolution doesn't start crashing because of referencing of freed memory).

2) Review appreciated. (I'm not yet very comfortable with the *_new() - g_object_unref() system).

Come on Paul, why is this in three separate patches? Didn't I ask (politely?) for one patch only? Why should I do one thing three times instead of only once? I do not see any reason why reviewing three patches instead of one. That all is still about the same bug and file anyway, and skipping a single patch will produce an error while applying the next, isn't it?

(In reply to comment #26)
> Come on Paul, why is this in three separate patches? Didn't I ask (politely?)
> for one patch only?

Because I didn't really understand this:
    "So we are waiting for a patch for evolution, one patch, which will cover
discovered issues on evolution side."

to mean something like:
    "Could you please provide one patch for all issues mentioned here?"

> Why should I do one thing three times instead of only once?
> I do not see any reason why reviewing three patches instead of one. That all is
> still about the same bug and file anyway, and skipping a single patch will
> produce an error while applying the next, isn't it?

I'm fine with providing one patch, of course. I can see how that would be easier for you to test. One question though: should I include "[RFC] plug leaking of objects", with which I'm less comfortable myself?

(In reply to comment #27)
> Because I didn't really understand this:
>     "So we are waiting for a patch for evolution, one patch, which will cover
> discovered issues on evolution side."
> 
> to mean something like:
>     "Could you please provide one patch for all issues mentioned here?"

OK, I'll try to be more clear next time.

> I'm fine with providing one patch, of course. I can see how that would be
> easier for you to test. One question though: should I include "[RFC] plug
> leaking of objects", with which I'm less comfortable myself?

I do not see anything wrong on that change. The camel_medium_set_content_object is calling camel_object_ref, thus it adds the reference, so unreffing is correct here. Same as camel_multipart_add_part adds a reference to the given part.
Good catch.

Created attachment 148410 [details] [review]
Plug leaks. Fix race.

One file, one bugreport, one patch. Patch plugs all leaks discussed here and fixes the race condition mentioned here too. Review appreciated.

Thanks for the update, patch looks good, also your suggested change to libytnef in comment #20 fixes crashing to me. Though running evolution under
valgrind --leak-check-full shows few issues:

==28624== Invalid read of size 4
==28624==    at 0x1BD3EAE9: SwapDWord (ytnef.c:130)
==28624==    by 0x1BD40330: TNEFPriority (ytnef.c:624)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==  Address 0x12e94a78 is 0 bytes inside a block of size 2 alloc'd
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD41A10: TNEFParse (ytnef.c:997)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==    by 0xEC1F87F: em_format_part_as (em-format.c:676)
==28624== 
==28624== Invalid read of size 1
==28624==    at 0x4A08310: memcpy (mc_replace_strmem.c:402)
==28624==    by 0x1BD3F02A: TNEFRendData (ytnef.c:255)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==  Address 0x1e65a2ce is 0 bytes after a block of size 14 alloc'd
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD41A10: TNEFParse (ytnef.c:997)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==    by 0xEC1F87F: em_format_part_as (em-format.c:676)
==28624== 
==28624== Invalid read of size 1
==28624==    at 0x4A08319: memcpy (mc_replace_strmem.c:402)
==28624==    by 0x1BD3F02A: TNEFRendData (ytnef.c:255)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==  Address 0x1e65a2cf is 1 bytes after a block of size 14 alloc'd
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD41A10: TNEFParse (ytnef.c:997)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==    by 0xEC1F87F: em_format_part_as (em-format.c:676)
/home/mcrha/.evolution/cache/tmp/tnef-attachment-dgdxMZ/calendar.vcf
==28624== 
==28624== Thread 11:
==28624== Invalid read of size 8
==28624==    at 0x1BB39AA4: saveVCalendar (tnef-plugin.c:906)
==28624==    by 0x1BB377A3: processTnef (tnef-plugin.c:242)
==28624==    by 0x1BB3725A: org_gnome_format_tnef (tnef-plugin.c:115)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==    by 0xEC1F87F: em_format_part_as (em-format.c:676)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==  Address 0x1b9ff298 is 0 bytes inside a block of size 4 alloc'd
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD3F723: TNEFFillMapi (ytnef.c:444)
==28624==    by 0x1BD3F210: TNEFMapiProperties (ytnef.c:322)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624== 
==28624== Invalid read of size 8
==28624==    at 0x1BD3EB01: SwapDDWord (ytnef.c:150)
==28624==    by 0x1BB3A303: saveVCalendar (tnef-plugin.c:1071)
==28624==    by 0x1BB377A3: processTnef (tnef-plugin.c:242)
==28624==    by 0x1BB3725A: org_gnome_format_tnef (tnef-plugin.c:115)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==    by 0xEC1F87F: em_format_part_as (em-format.c:676)
==28624==  Address 0x17fee2f8 is 0 bytes inside a block of size 4 alloc'd
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD3F723: TNEFFillMapi (ytnef.c:444)
==28624==    by 0x1BD3F210: TNEFMapiProperties (ytnef.c:322)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624== 
==28624== Invalid read of size 8
==28624==    at 0x1BB3A329: saveVCalendar (tnef-plugin.c:1073)
==28624==    by 0x1BB377A3: processTnef (tnef-plugin.c:242)
==28624==    by 0x1BB3725A: org_gnome_format_tnef (tnef-plugin.c:115)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)
==28624==    by 0xEC1F87F: em_format_part_as (em-format.c:676)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==  Address 0x17fee2f8 is 0 bytes inside a block of size 4 alloc'd
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD3F723: TNEFFillMapi (ytnef.c:444)
==28624==    by 0x1BD3F210: TNEFMapiProperties (ytnef.c:322)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)

==28624== 0 bytes in 2 blocks are definitely lost in loss record 1 of 303
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD3F618: TNEFFillMapi (ytnef.c:419)
==28624==    by 0x1BD3F1CC: TNEFAttachmentMAPI (ytnef.c:316)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)

==28624== 150 (32 direct, 118 indirect) bytes in 2 blocks are definitely lost in loss record 102 of 303
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD3F349: TNEFFillMapi (ytnef.c:364)
==28624==    by 0x1BD3F210: TNEFMapiProperties (ytnef.c:322)
==28624==    by 0x1BD41C05: TNEFParse (ytnef.c:1026)
==28624==    by 0x1BD414BA: TNEFParseFile (ytnef.c:890)
==28624==    by 0x1BB37233: org_gnome_format_tnef (tnef-plugin.c:112)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624== 
==28624== 
==28624== 208 bytes in 1 blocks are definitely lost in loss record 120 of 303
==28624==    at 0x4A05414: calloc (vg_replace_malloc.c:397)
==28624==    by 0x1BD42B97: DecompressRTF (ytnef.c:1330)
==28624==    by 0x1BB39F23: saveVCalendar (tnef-plugin.c:1004)
==28624==    by 0x1BB377A3: processTnef (tnef-plugin.c:242)
==28624==    by 0x1BB3725A: org_gnome_format_tnef (tnef-plugin.c:115)
==28624==    by 0x18D76144: plugin_lib_invoke (e-plugin-lib.c:94)
==28624==    by 0x645ED84: e_plugin_invoke (e-plugin.c:688)
==28624==    by 0xE7921F6: emfh_format_format (em-format-hook.c:75)
==28624==    by 0xE799F70: efhd_format_attachment (em-format-html-display.c:466)
==28624==    by 0xEC1F8DE: em_format_part_as (em-format.c:686)
==28624==    by 0xEC1F976: em_format_part (em-format.c:705)
==28624==    by 0xEC21746: emf_multipart_mixed (em-format.c:1436)

Note I compiled my own libytnef from a source package [1] and patched it to not crash for me. I selected 3 different email with TNEF attachments you gave me a link to. It's possible that some leaks are caused by libytnef itself, I didn't check that. I would like to ask you first:
Do you see these valgrinds too?

[1] http://kojipkgs.fedoraproject.org/packages/libytnef/1.5/4.fc11/src/libytnef-1.5-4.fc11.src.rpm

Looking more closely to the above valgrind reports I would say those are ytnef issues, not the evolution's plugin, thus far out of scope for this bug.
Thanks for a patch Paul, please commit it to master.

(In reply to comment #31)
> Looking more closely to the above valgrind reports I would say those are ytnef
> issues, not the evolution's plugin, thus far out of scope for this bug.

Indeed there seem to be some (potential) leaks.

OT: what are those "invalid reads"? 

> Thanks for a patch Paul, please commit it to master.

Hope to do so soon. And thanks for the review.

(In reply to comment #32)
> OT: what are those "invalid reads"? 

If I recall correctly the uninitialized memory is shown differently, thus most likely reading from an already freed memory.

> > Thanks for a patch Paul, please commit it to master.
> 
> Hope to do so soon. And thanks for the review.

The best before Monday release.

Commit dae1d43302906876882c153c40a0e7d45f66023d (http://git.gnome.org/cgit/evolution/commit/?id=dae1d43302906876882c153c40a0e7d45f66023d ). Resolved / Fixed.