210109 - GetInt() can crash mozilla and thunderbird on misaligned address assignment

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Mitch]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.5a) Gecko/20030611
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.5a) Gecko/20030611

Clicking in the search box in the mail window frontend and typing any text
will crash mozilla 100% reproduceably.

Stripped stack:

[New LWP 8]

Program received signal SIGSEGV, Segmentation fault.
0xff23fc9c in PR_vsprintf_append ()
   from /share/cprbld/users/scratch/mitch/mozilla/mozilla/dist/bin/libnspr4.so
(gdb) bt
#0  0xff23fc9c in PR_vsprintf_append ()
   from /share/cprbld/users/scratch/mitch/mozilla/mozilla/dist/bin/libnspr4.so
etc..

I am rebuilding a debug version now to provide a more exact stack.

Reproducible: Always

Steps to Reproduce:
1. Search for anything in the window frontend search box.
2.
3.

Actual Results:  
Mozilla crashes

Comment 1

[Mitch]

Attachment: gdb stack trace from a search

Stack trace of the crash as promised.

Comment 2

[Mitch]

Ok, i've had some time to do some more work on this.
It appears that we'll only see this on Solaris where we have strict
alignments. We're actually getting a SIGBUS:

Program received signal SIGBUS, Bus error.
0xff0bfc9c in GetInt (state=0xffbfdb20, code=88) at prscanf.c:324
324                         *va_arg(state->ap, PRUint32 *) = lval;
(gdb) p lval                                                      
$15 = 7     
(gdb) p *state
$10 = {get = 0xff0c0a28 <StringGetChar>, unget = 0xff0c0a54 <StringUngetChar>,
  stream = 0xffbfdb9c, ap = 0xffbfdba8, nChar = 2, assign = 1, width = -1,    
  sizeSpec = _PR_size_none, converted = 0}                                
                                          
(gdb) info registers                      
g0             0x0      0
g1             0xe1000  921600
g2             0x894991 8997265
g3             0x894990 8997264
g4             0x4b8    1208   
g5             0x0      0   
g6             0x0      0
g7             0xfead0000       -22216704
o0             0x840d59 8654169          
o1             0xffbfdba8       -4203608
o2             0x7      7               
o3             0x37     55
o4             0xf      15
o5             0x0      0 
...                      
pc             0xff0bfc9c       -15991652
npc            0xff0bfca0       -15991648
fpsr           0x620    1568             
cpsr           0x0      0   
(gdb)                    
     
Looking at the %pc
                  
(gdb) x 0xff0bfc9c
0xff0bfc9c <GetInt+916>:        st  %o2, [ %o0 ]

You can see that %o0 = 0x840d59 which is misaligned.

So basically we're assigning a 32 bit val to a misaligned
address.
324                         *va_arg(state->ap, PRUint32 *) = lval;

I've put my thinking hat on on how we can do this correctly if someone
doesn't beat me to it. Note, that i can also reproduce this on thunderbird
and mozilla-mail when it encounters a subject line with ISO-8859-1 encoding.

I'm changing the Summary line to be more appropriate.

Comment 3

[Mitch]

Attachment: Patch

Further tracking down shows that DecodeQ() calls PR_sscanf() that
eventually calls GetInt() and the va_args() argument is a char *
which can be misaligned, but we attempt to store an int32 value to
a misaligned address. Since DecodeQ() always sscanf()'s for a char
really, we shouldn't be calling GetInt() and attempting to store into
an aligned address. This patch ensure's we are called with the argument
to PR_sscanf() as an 'int' and then assign it to a 'char'.

Can someone review please and tell me how to integrate (it's my first time).

Comment 4

[Mitch]

Attachment: new proposed patch

Damn, looks like i forgot to cast the into to a char since the
assmbler produced would write a word instead of a byte to the
location *out

Comment 5

[Mitch]

Attachment: new proposed patch

They do say "good things come in three's", so here's take 3:

Got another SEGV with the old patch in:
Program received signal SIGSEGV, Segmentation fault.
0xff0bfc9c in GetInt (state=0xffbfdb20, code=88) at prscanf.c:324
324			    *va_arg(state->ap, PRUint32 *) = lval;

Turns out i'm passing the value instead of the address of 'c' to PR_sscanf(),
doh !!

This new patch initializes the variable 'c' and passes it as an address to
PR_sscanf() !

Tested this again and it now appears stable.

Comment 6

[Mitch]

More digging has uncovered that this bug started happening since the
checkin for 1.117 mailnews/mime/src/comi18n.cpp by jshin for bug 162765
on 12th June.

The change in MIME_DecodeMimeHeader() function now calls
DecodeRFC2047Header() that traverses this codepath up to
GetInt() causing the SIGBUS.

Again this only happens on Sparc platforms with strich alignment.
The proposed patch has little/no affect to non-sparc platforms, so
am looking at the netlib reviewers to check this code. I'm more than
willing to do the checkin if someone lets me know how to or points me
to a page describing this.

I've change this to blocker since we can't do any more testing of
mozilla on Solaris/Sparc platform.

Comment 7

[Jungshik Shin]

Thank you for spotting this problem. Some other RISC processors might have the
same problem. 

I'm fine with the patch. Anyway, I'm adding some people to CC.
wtc, am I not supposed to use 'char' in PR_sscanf?  

Comment 8

[Wan-Teh Chang]

Comment on attachment 126382 [details] [diff] [review]
new proposed patch

r=wtc.

The %X format must match a PRUintn.  (PRUintn and
PRIntn are the same size, so it is fine to use them
interchangeably here.)

The original code not only writes to a potentially
misaligned address but also writes more bytes than it
should.

As a matter of style, it is better to declare 'c' as
a PRUintn or PRIntn.  You can make this change when
you check in this patch.

Comment 9

[Jungshik Shin]

Comment on attachment 126382 [details] [diff] [review]
new proposed patch

wtc, thanks for your quick help. So, it was by a sheer luck that my code worked
(I'm wondering whether it ever worked on any BE machines.). 

alecf, can you sr? I'll change 'int c' to 'PRUint32 c'.

Comment 10

[Wan-Teh Chang]

Don't use PRUint32. PRUint32 is not necessarily the same
size as PRUintn, which is the most correct type matching
the %2X format.  (int and PRIntn are the same size as
PRUintn, which is why they are okay.)

Comment 11

[Darin Fisher]

Comment on attachment 126382 [details] [diff] [review]
new proposed patch

sr=darin

Comment 12

[Jungshik Shin]

Thank you all for the patch (I'm very sorry I just realized that I forgot to
credit Mitch with the patch when landing it), r/sr and help. Fix was checked in
with PRUintn in place of int.