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

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: David Volgyes, Assigned: Atul Aggarwal)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general

Updated

6 years ago
Blocks: 679417
(Assignee)

Comment 1

6 years ago
Created attachment 556392 [details] [diff] [review]
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.
Attachment #556392 - Flags: review?(timeless)

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

6 years ago
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+
(Assignee)

Comment 3

6 years ago
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!

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

Comment 5

6 years ago
@Steve Thanks for the check-in. This is my first patch in mozilla :).
http://hg.mozilla.org/mozilla-central/rev/0cf9208a2bb5

Congratulations on the first patch making the product then!
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Assignee)

Comment 7

6 years ago
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 :).

Updated

6 years ago
Assignee: general → atulagrwl
You need to log in before you can comment on or make changes to this bug.