678988 - potential null pointer dereference in js/jsd/jsd_scpt.c

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Volgyes]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

cppcheck 1.49 (http://cppcheck.sourceforge.net/) found a plenty of potential null pointer dereference. This is one of them.




Actual results:

There is a check in line #533 (if LIVEWIRE is defined) for jsdscript!=0. 
So jsdscript definitely can be NULL.
But a few lines below (line #543) there is a jsdscript->script dereference, where jsdscript!=0 condition is not guaranteed.

#ifdef LIVEWIRE
    if( jsdscript && jsdscript->lwscript )
    {
        uintN newline;
        jsdlw_RawToProcessedLineNumber(jsdc, jsdscript, line, &newline);
        if( line != newline )
            line = newline;
    }
#endif

    call = JS_EnterCrossCompartmentCallScript(jsdc->dumbContext, jsdscript->script);



Expected results:

You should check jsdscript pointer, and handle somehow if it is null.

Comment 1

[Atul Aggarwal]

Attachment: v1 patch to null check jsdscript

A simple fix which null check the jsdscript and return 0 in case any of them is null.
timeless, please pass the review to another person if I have wrongly assigned it to you. I found your name by looking at logs of jsd_scpt.c file.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 556392 [details] [diff] [review]
v1 patch to null check jsdscript

I'll poach this review.

nit: the spacing should match the (horrible) spacing in the surrounding context:

  if( !jsdscript )

Either that, or you can reformat the whole file with reasonable spacing ("if (!jsdscript) {"), but that should be done in a separate bug or at least a separate patch. (So don't bother, unless you're feeling especially motivated, but be aware that this code will hopefully die soon anyway.)

Comment 3

[Atul Aggarwal]

Do I need to upload the updated patch? I would not attempt to reformat the complete file if this code is going to die soon. If this code is going to live, i can attempt to reformat the file. This way I will learn the coding style also.
I guess the problem which we need to fix in this file is "if(condition1 || condition2) {".

Comment 4

[Steve Fink [:sfink] [:s:]]

Oh, you're going to checkin? anyway. Ok, I fixed the nit and landed the patch. Thanks!

http://hg.mozilla.org/integration/mozilla-inbound/rev/0cf9208a2bb5

Comment 5

[Atul Aggarwal]

@Steve Thanks for the check-in. This is my first patch in mozilla :).

Comment 6

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/0cf9208a2bb5

Congratulations on the first patch making the product then!

Comment 7

[Atul Aggarwal]

Thanks a lot mak. This is just the starting. I have already submitted 10 more patches and waiting for them to get into mozilla-central :).