Last Comment Bug 362735 - 2 problems in MimeRichtextConvert
: 2 problems in MimeRichtextConvert
Status: RESOLVED FIXED
[sg:critical]
: fixed1.8.0.10, fixed1.8.1.1
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: ---
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-04 08:29 PST by georgi - hopefully not receiving bugspam
Modified: 2008-07-31 04:30 PDT (History)
2 users (show)
dveditz: blocking1.8.0.10+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
perl to generate mail folder on 64 bit systems (434 bytes, text/plain)
2006-12-04 08:32 PST, georgi - hopefully not receiving bugspam
no flags Details
perl to generate mail folder causing negative size on 32 bit system (453 bytes, text/plain)
2006-12-04 08:34 PST, georgi - hopefully not receiving bugspam
no flags Details
proposed patch (1.29 KB, patch)
2006-12-06 07:49 PST, georgi - hopefully not receiving bugspam
mscott: superreview+
Details | Diff | Splinter Review
proposed patch, addresses comments (1.25 KB, patch)
2006-12-06 23:57 PST, georgi - hopefully not receiving bugspam
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2006-12-04 08:29:38 PST
2 problems in MimeRichtextConvert 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/mimetric.cpp&rev=1.16
106 seawood  1.13   // The code below must never expand the input by more than 5x;

107 mstoltz  1.12   // if it does, the desired_size multiplier (5) below must be changed too
108                 [1] desired_size = (length * 5) + 1;

109 rhp      1.1    [2] if (desired_size >= *obuffer_sizeP)
110               	status = mime_GrowBuffer (desired_size, sizeof(char), 1024,
111               							 obufferP, obuffer_sizeP);

so 2 problems:

A) |desired_size| may be negative even on 32 bit systems at the cost of 
1.8G VM (~ 425M length).
the check at [2] is useless in this case.

B) on 64 bit systems [1] may overflow at the cost of about 3.6G VM 
(~ 850M length). this may work in theory on 32 bit systems, but could reproduce it only on a 64 bit system.

stack from the negative case:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1223984304 (LWP 4848)]
0xb739ec33 in strcpy () from /lib/i686/libc.so.6
(gdb) bt
#0  0xb739ec33 in strcpy () from /lib/i686/libc.so.6
#1  0xb7cc8311 in PL_strcpy (
    dest=0x8bd7ffd "&am" <Address 0x8bd8000 out of bounds>, 
    src=0xb4c0faa8 "&amp;")
    at /opt/joro/thunderbird20/mozilla/nsprpub/lib/libc/src/strcpy.c:46
#2  0xb4b885c8 in MimeRichtextConvert (
    line=0x70417008 '&' <repeats 200 times>..., length=445644801, 
    output_fn=0xb4b8ad74 <mime_output_fn>, closure=0x8b98020, 
    obufferP=0x8a76e04, obuffer_sizeP=0x8a76e0c, enriched_p=1)
    at /opt/joro/thunderbird20/mozilla/mailnews/mime/src/mimetric.cpp:167
#3  0xb4b89036 in MimeInlineTextRichtext_parse_line (
    line=0x70417008 '&' <repeats 200 times>..., length=445644801, 
(gdb) frame 2
#2  0xb4b885c8 in MimeRichtextConvert (
    line=0x70417008 '&' <repeats 200 times>..., length=445644801, 
    output_fn=0xb4b8ad74 <mime_output_fn>, closure=0x8b98020, 
    obufferP=0x8a76e04, obuffer_sizeP=0x8a76e0c, enriched_p=1)
    at /opt/joro/thunderbird20/mozilla/mailnews/mime/src/mimetric.cpp:167
167                       PL_strcpy (out, "&amp;"); out += strlen (out);
(gdb) x/4c out
0x8bd7ffd:      38 '&'  97 'a'  109 'm' Cannot access memory at address 0x8bd8000
(gdb) x/4c out-20
0x8bd7fe9:      38 '&'  97 'a'  109 'm' 112 '
Comment 1 georgi - hopefully not receiving bugspam 2006-12-04 08:32:23 PST
Created attachment 247421 [details]
perl to generate mail folder on 64 bit systems
Comment 2 georgi - hopefully not receiving bugspam 2006-12-04 08:34:20 PST
Created attachment 247422 [details]
perl to generate mail folder causing negative size on 32 bit system
Comment 3 georgi - hopefully not receiving bugspam 2006-12-06 07:49:01 PST
Created attachment 247683 [details] [diff] [review]
proposed patch

proposed patch. |new| still throws, but this is another bug.
Comment 4 David :Bienvenu 2006-12-06 10:30:54 PST
Comment on attachment 247683 [details] [diff] [review]
proposed patch

seems like it might be clearer to write something like:

0xfffffffe / BGROWTH

and it looks like there's a tab before the return -1.

r=bienvenu, with those nits. thx for the patch!
Comment 5 Scott MacGregor 2006-12-06 12:49:33 PST
Comment on attachment 247683 [details] [diff] [review]
proposed patch

including David's suggestions...
Comment 6 georgi - hopefully not receiving bugspam 2006-12-06 23:57:51 PST
Created attachment 247793 [details] [diff] [review]
proposed patch, addresses comments

proposed patch, addresses comments
Comment 7 georgi - hopefully not receiving bugspam 2006-12-08 00:18:04 PST
imho this blocks both branches and with favorable heap layout is exploitable on 32 bit os - classic under allocation without exhausting the process memory space.

don't have cvs access so can't check in this.
Comment 8 georgi - hopefully not receiving bugspam 2006-12-13 07:55:23 PST
bz, this seems to be blocking both branches.
Comment 9 Boris Zbarsky [:bz] 2006-12-13 08:08:15 PST
So request blocking.  ;)  ccing me on mailnews innards bugs is not really useful, though -- I rarely plan to do anything about them.
Comment 10 David :Bienvenu 2006-12-13 08:26:55 PST
fixed on trunk and 1.8.1 branch - thx, G30rgi
Comment 11 georgi - hopefully not receiving bugspam 2006-12-13 08:27:45 PST
(In reply to comment #9)
> So request blocking.  ;)  ccing me on mailnews innards bugs is not really
> useful, though -- I rarely plan to do anything about them.
> 

ok, you are not gonna be spammed about mailnews anymore :)

i meant someone empowered to request blocking.
Comment 12 Daniel Veditz [:dveditz] 2006-12-18 14:20:27 PST
FIXED on trunk, blocking flags track the branch
Comment 13 Scott MacGregor 2007-01-08 14:13:47 PST
Comment on attachment 247793 [details] [diff] [review]
proposed patch, addresses comments

requesting approval for 1.8.0.10 since this is listed as a blocker. It's on the trunk and the 1.8.1 branch baking for a while.
Comment 14 Jay Patel [:jay] 2007-01-08 15:50:29 PST
Comment on attachment 247793 [details] [diff] [review]
proposed patch, addresses comments

Approved for 1.8.0 branch, a=jay for drivers.
Comment 15 David :Bienvenu 2007-01-15 15:30:28 PST
fixed on 1.8.0.x branch

Note You need to log in before you can comment on or make changes to this bug.