352470 - uninitialized memory read in SrcNoteForPropOp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Moening]

In SrcNoteForPropOp has garbage memory in it.

The code is shown below. 
In this code the "pn->pn_attrs" member,
which itself is a define actually pointing to:

pn->pn_u->name->attrs

has not been intialized at the time of this call.
It has the value of:  4227595259
The slot member is bad too:  slot =  -67372037 

-----------------
static jssrcnote
SrcNoteForPropOp(JSParseNode *pn, JSOp op)
{
    return ((op == JSOP_GETMETHOD &&
             !(pn->pn_attrs & JSPROP_IMPLICIT_FUNCTION_NAMESPACE)) ||
            op == JSOP_SETMETHOD)
           ? SRC_PCDELTA
           : SRC_PCBASE;
}
-----------------

Here is the pn_u member layed out:
----------------------------------
pn_u    {func={...} list={...} ternary={...} ...}      
<unnamed-tag>
+               func    {funAtom=0x03bd9090 body=0x03bdec30 flags=4227595259
..}       <unnamed-tag>
+               list    {head=0x03bd9090 tail=0x03bdec30 count=4227595259 ...} 
<unnamed-tag>
+               ternary {kid1=0x03bd9090 kid2=0x03bdec30 kid3=0xfbfbfbfb }     
<unnamed-tag>
+               binary  {left=0x03bd9090 right=0x03bdec30 val=-67372037 }      
<unnamed-tag>
+               unary   {kid=0x03bd9090 num=62778416 }  <unnamed-tag>
-               name    {atom=0x03bd9090 expr=0x03bdec30 slot=-67372037 ...}   
<unnamed-tag>
+               atom    0x03bd9090 {entry={...} flags=0 number=376 }    JSAtom
*
+               expr    0x03bdec30 {pn_type=29 pn_op=';' pn_arity='ÿ' ...}     
JSParseNode *
                slot    -67372037       long
                attrs   4227595259      unsigned int
--------------------------

Here's the call stack to get here:
---------------------------------------------
>	js32d.dll!SrcNoteForPropOp(JSParseNode * pn=0x03bdec58, JSOp op=JSOP_GETMETHOD)  Line 2283 + 0x3d bytes	C
        js32d.dll!EmitPropOp(JSContext * cx=0x03bb87b0, JSParseNode *
pn=0x03bdec58, JSOp op=JSOP_GETMETHOD, JSCodeGenerator * cg=0x0439eca0)  Line
2349 + 0xf6 bytes   C
        js32d.dll!js_EmitTree(JSContext * cx=0x03bb87b0, JSCodeGenerator *
cg=0x0439eca0, JSParseNode * pn=0x03bdec58)  Line 5659 + 0x9e bytes  C
        js32d.dll!js_EmitTree(JSContext * cx=0x03bb87b0, JSCodeGenerator *
cg=0x0439eca0, JSParseNode * pn=0x03bdec80)  Line 5698 + 0x77 bytes  C
        js32d.dll!js_EmitTree(JSContext * cx=0x03bb87b0, JSCodeGenerator *
cg=0x0439eca0, JSParseNode * pn=0x03bdee18)  Line 5056 + 0x77 bytes  C
        js32d.dll!Statements(JSContext * cx=0x03bb87b0, JSTokenStream *
ts=0x03bde8b0, JSTreeContext * tc=0x0439eca0)  Line 1472 + 0x17e bytes  C
        js32d.dll!js_CompileTokenStream(JSContext * cx=0x03bb87b0, JSObject *
chain=0x03b970e8, JSTokenStream * ts=0x03bde8b0, JSCodeGenerator *
cg=0x0439eca0)  Line 501 + 0x77 bytes  C
        js32d.dll!CompileTokenStream(JSContext * cx=0x03bb87b0, JSObject *
obj=0x03b970e8, JSTokenStream * ts=0x03bde8b0, void * tempMark=0x03bb87f8, int
* eofp=0x00000000)  Line 3848 + 0x7e bytes    C
        js32d.dll!JS_CompileUCScriptForPrincipals(JSContext * cx=0x03bb87b0,
JSObject * obj=0x03b970e8, JSPrincipals * principals=0x00000000, const unsigned
short * chars=0x03ba0d90, unsigned int length=29024, const char *
filename=0x03bde578, unsigned int lineno=1)  Line 3943 + 0x9f bytes      C
        js32d.dll!JS_CompileUCScript(JSContext * cx=0x03bb87b0, JSObject *
obj=0x03b970e8, const unsigned short * chars=0x03ba0d90, unsigned int
length=29024, const char * filename=0x03bde578, unsigned int lineno=1)  Line
3911 + 0xc5 bytes C
---------------------------------------------

Lastly heres the entire pn struct.
(I expanded the pn_ts so you could see what was being parsed).
---------------------------------------------------------

-               pn      0x03bdec58 {pn_type=22 pn_op='¸' pn_arity='ÿ' ...}     
JSParseNode *
                pn_type 22      unsigned short
                pn_op   184 '¸' unsigned char
                pn_arity        -1 'ÿ'  char
+               pn_pos  {begin={...} end={...} }        JSTokenPos
                pn_offset       0       int
+               pn_u    {func={...} list={...} ternary={...} ...}      
<unnamed-tag>
+               pn_next 0x03bdedf0 {pn_type=31 pn_op='=' pn_arity=0 ...}       
JSParseNode *
-               pn_ts   0x03bde8b0 {tokens=0x03bde8b0 cursor=3 lookahead=0 ...}
JSTokenStream *
+               tokens  0x03bde8b0 {type=TOK_LP pos={...} ptr=0x03bde9a4
"("<!DOCTYPE HTML PUBLIC>\r\n<html>\r\n<head>\r\n");
" ...}  JSToken [4]
                cursor  3       unsigned int
                lookahead       0       unsigned int
                lineno  2       unsigned int
                ungetpos        0       unsigned int
+               ungetbuf        0x03bde920 "("  unsigned short [6]
                flags   168     unsigned int
                linelen 66      int
                linepos 0       int
+               linebuf {base=0x03bde988 "response.write("<!DOCTYPE HTML
PUBLIC>\r\n<html>\r\n<head>\r\n");
" limit=0x03bdea0c "" ptr=0x03bdea0a "
" }     JSTokenBuf
+               userbuf {base=0x03ba0d90 "
response.write("<!DOCTYPE HTML PUBLIC>\r\n<html>\r\n<head>\r\n");

response.write("\r\n\r\n");



 if(!page.JSON)
 {
   JSON = 
   {
     _fixStringReplacements :  { '"'  : '\\"', "\\" : "\\\\", "\n" : "\\n",
"\r" : "\\r", "\b" : "\\b", "\t" : "\\t", "\f" : "\\f" },  

     toJSONString : function(oObj)
     {
       var str = "";
       switch(typeof(oObj))
       {
         case "boolean":
         case "number":
           return oObj;
         case "string":
           if(/[\" limit=0x03baf050 "" ptr=0x03ba0e16 "
response.write("\r\n\r\n");



 if(!page.JSON)
 {
   JSON = 
   {
     _fixStringReplacements :  { '"'  : '\\"', "\\" : "\\\\", "\n" : "\\n",
"\r" : "\\r", "\b" : "\\b", "\t" : "\\t", "\f" : "\\f" },  

     toJSONString : function(oObj)
     {
       var str = "";
       switch(typeof(oObj))
       {
         case "boolean":
         case "number":
           return oObj;
         case "string":
           if(/[\"\\\x00-\x1f]/.test(oObj       JSTokenBuf
+               tokenbuf        {base=0x03bdebb0 "<!DOCTYPE HTML PUBLIC>
<html>
<head>
&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#65339;"
limit=0x03bdec2e "&#64507;&#65339;" ptr=0x03bdec00
"&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#64507;&#65339;"
..} JSStringBuffer
+               filename        0x03bde578 "via/shapes.mwsp"    const char *
+               file    0x00000000 {_ptr=??? _cnt=??? _base=??? ...}    _iobuf
*
+               principals      0x00000000 {codebase=??? getPrincipalArray=???
globalPrivilegesEnabled=??? ...} JSPrincipals *
                listener        0x00000000      void (const char *, unsigned
int, unsigned short *, unsigned int, void * *, void *)*
                listenerData    0x00000000      void *
                listenerTSData  0x00000000      void *
+               saveEOL 0x00000000 <Bad Ptr>    unsigned short *


-----------------------

Comment 1

[Brian Crowder]

Attachment: Fix UMR

MikeM:  Can you test this patch out, please?  It initializes the pn_attrs for the node you're having trouble with, I believe.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 238212 [details] [diff] [review]
Fix UMR

Set pn_slot = -1 too.  No more uninitialized members for a given pn_arity (I was a slacker, remember? :-)

/be

Comment 3

[Mike Moening]

What about the slot member?

   pn->pn_u->name->slot

It need initialization too.

Otherwise the original patch fixed the error.
Thanks!

Comment 4

[Brian Crowder]

Attachment: Fix UMR more

Man, I can't believe what a slacker Brendan is...

Someone less slackerly than Brendan and more qualified than me might want to consider auditing some of these other nodes to see if they are left uninitialized in important ways (or we should consider running purify or bounds-checker, or some other such goodness against our testsuite/fuzzer(s) to see what badness they may find.)

MikeM:  When you get time, please give this new patch a go.

Comment 5

[Mike Moening]

That patch is fine too.

Comment 6

[Brian Crowder]

Comment on attachment 238233 [details] [diff] [review]
Fix UMR more

Should I nominate this for approval in any branches?

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 238233 [details] [diff] [review]
Fix UMR more

>Index: jsparse.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsparse.c,v
>retrieving revision 3.244
>diff -u -8 -p -r3.244 jsparse.c
>--- jsparse.c	12 Sep 2006 21:56:32 -0000	3.244
>+++ jsparse.c	13 Sep 2006 17:41:41 -0000
>@@ -4276,16 +4276,18 @@ MemberExpr(JSContext *cx, JSTokenStream 
>             tt = pn3->pn_type;
>             if (tt == TOK_NAME ||
>                 (tt == TOK_DBLCOLON &&
>                  pn3->pn_arity == PN_NAME &&
>                  pn3->pn_expr->pn_type == TOK_FUNCTION)) {
>                 pn2->pn_op = (tt == TOK_NAME) ? JSOP_GETPROP : JSOP_GETMETHOD;
>                 pn2->pn_expr = pn;
>                 pn2->pn_atom = pn3->pn_atom;
>+                pn2->pn_attrs = 0;
>+                pn2->pn_slot = -1;

Hard to believe, but this code goes back to when PN_NAME had no pn_slot or pn_attrs members (they're declared in that order, so I try to init them in that order too; nit for this patch).

Searching for NewParseNode.*PN_NAME, I see ImportExpr for the 'import foo.*' case, the TOK_DOT case in MemberExpr (set pn_slot and pn_attrs right away, I say!), and both TOK_LEXICALSCOPE constructor cases fail to set all members of the pn_u.name union arm.

Can you patch these too?  Thanks.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> >@@ -4276,16 +4276,18 @@ MemberExpr(JSContext *cx, JSTokenStream 
> >             tt = pn3->pn_type;
> >             if (tt == TOK_NAME ||
> >                 (tt == TOK_DBLCOLON &&
> >                  pn3->pn_arity == PN_NAME &&
> >                  pn3->pn_expr->pn_type == TOK_FUNCTION)) {
> >                 pn2->pn_op = (tt == TOK_NAME) ? JSOP_GETPROP : JSOP_GETMETHOD;
> >                 pn2->pn_expr = pn;
> >                 pn2->pn_atom = pn3->pn_atom;
> >+                pn2->pn_attrs = 0;
> >+                pn2->pn_slot = -1;
> 
> Hard to believe, but this code goes back to when PN_NAME had no pn_slot or
> pn_attrs members

I meant by "this code" all of JSParseNode and jsparse.[ch], not this particular paragraph.  My slackerness arose when adding pn_slot and pn_attrs but setting them only for those cases that (at first) needed them.  Lazy, impatient, hubristic -- why am I not a Perl hacker?

The Greek master playwrights taught that from hubris comes nemesis and one's own destruction.  They didn't know about UMRs though ;-).

/be

Comment 9

[Brian Crowder]

Attachment: Fix potential UMRs more, and pick some nits

At or about line 3077 of jsparse.c there is a pn_slot being initialized to 0.  Should that be -1?

I moved up the initial two-liner to just after pn2 is built to catch all of these cases (as I think Brendan is recommending when he says "right away"), and added inits for these to all the recommended spots.

I'll go another round on reviews if this makes Brendan happy.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #9)
> At or about line 3077 of jsparse.c there is a pn_slot being initialized to 0. 
> Should that be -1?

That's the case where we're setting up a catch variable in its enclosing lexical scope. At that point, we know that the parse node will be at slot 0, since nothing else can occupy that slot in its parent block object. So that one's OK.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Created an attachment (id=238241) [edit]
> Fix more UMR more, and pick some nits
> 
> At or about line 3077 of jsparse.c there is a pn_slot being initialized to 0. 
> Should that be -1?

No, that's slot 0 in the catch block, which has exactly one local for the catch (e) anc catch(e if ...) cases (destructuring catch could use more locals, but that's handled by the other cases in the switch containing this case).

> I moved up the initial two-liner to just after pn2 is built to catch all of
> these cases (as I think Brendan is recommending when he says "right away"), and
> added inits for these to all the recommended spots.

Thanks.  The ImportExpr case for foo.* should set pn_expr = NULL too.  Sorry I didn't notice that earlier.

> I'll go another round on reviews if this makes Brendan happy.

I'm never happy, don't worry about me.

At some point we want to consider time/code-space/maintainability trade-offs and maybe have a NewNameNode wrapper that takes parameters for all the members.  Not for 1.8.1.

/be

Comment 12

[Brian Crowder]

ImportExpr is okay, since pn2->pn_expr = pn; happens after both conditions?

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> ImportExpr is okay, since pn2->pn_expr = pn; happens after both conditions?

Duh, should have read the source, inferred that, or remembered it.  Wasn't in the context, and I'm getting context-diff tunnel vision from all this fuzz-fixing and reviewing :-/.

Stamping in a minute, thanks for doing this janitorial work.

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 238241 [details] [diff] [review]
Fix potential UMRs more, and pick some nits

Looks good, and I think mrbkap likes it.  I'll land it on the trunk.  Nominating for 1.8.1.

Thanks to MikeM for finding this, and to crowder for patching!

/be

Comment 15

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 16

[Brendan Eich [:brendan]]

UMRs are hazardous.  This one does not appear to be exploitable, but safety first.

/be

Comment 17

[Mike Schroepfer]

Comment on attachment 238241 [details] [diff] [review]
Fix potential UMRs more, and pick some nits

a=schrep for 181drivers.