Closed Bug 678988 Opened 11 years ago Closed 11 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: david.volgyes, Assigned: atulagrwl)




(1 file)

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 ( 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.

    if( jsdscript && jsdscript->lwscript )
        uintN newline;
        jsdlw_RawToProcessedLineNumber(jsdc, jsdscript, line, &newline);
        if( line != newline )
            line = newline;

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

Expected results:

You should check jsdscript pointer, and handle somehow if it is null.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Blocks: cppcheck
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.
Attachment #556392 - Flags: review?(timeless)
Ever confirmed: true
Attachment #556392 - Flags: review?(brendan)
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.)
Attachment #556392 - Flags: review?(timeless)
Attachment #556392 - Flags: review?(brendan)
Attachment #556392 - Flags: review+
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) {".
Oh, you're going to checkin? anyway. Ok, I fixed the nit and landed the patch. Thanks!
@Steve Thanks for the check-in. This is my first patch in mozilla :).

Congratulations on the first patch making the product then!
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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 :).
Assignee: general → atulagrwl
You need to log in before you can comment on or make changes to this bug.