Last Comment Bug 678988 - potential null pointer dereference in js/jsd/jsd_scpt.c
: potential null pointer dereference in js/jsd/jsd_scpt.c
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- normal (vote)
: mozilla9
Assigned To: Atul Aggarwal
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: cppcheck
  Show dependency treegraph
Reported: 2011-08-15 07:47 PDT by David Volgyes
Modified: 2011-08-31 02:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 patch to null check jsdscript (666 bytes, patch)
2011-08-28 07:48 PDT, Atul Aggarwal
sphink: review+
Details | Diff | Splinter Review

Description User image David Volgyes 2011-08-15 07:47:30 PDT
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.
Comment 1 User image Atul Aggarwal 2011-08-28 07:48:30 PDT
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.
Comment 2 User image Steve Fink [:sfink] [:s:] 2011-08-29 14:08:23 PDT
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 User image Atul Aggarwal 2011-08-29 20:56:00 PDT
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 User image Steve Fink [:sfink] [:s:] 2011-08-30 12:09:32 PDT
Oh, you're going to checkin? anyway. Ok, I fixed the nit and landed the patch. Thanks!
Comment 5 User image Atul Aggarwal 2011-08-30 12:38:50 PDT
@Steve Thanks for the check-in. This is my first patch in mozilla :).
Comment 6 User image Marco Bonardo [::mak] 2011-08-31 02:05:36 PDT

Congratulations on the first patch making the product then!
Comment 7 User image Atul Aggarwal 2011-08-31 02:15:12 PDT
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 :).

Note You need to log in before you can comment on or make changes to this bug.