Bug 34730 - [Qt] Null QObjects properties cause Segmentation Fault
Summary: [Qt] Null QObjects properties cause Segmentation Fault
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-02-08 15:57 PST by Bruno Schmidt
Modified: 2014-04-24 16:45 PDT (History)
8 users (show)

See Also:


Attachments
Patch fixing the bug (2.02 KB, patch)
2010-02-08 15:57 PST, Bruno Schmidt
no flags Details | Formatted Diff | Diff
Bug Test Program (1009 bytes, application/zip)
2010-03-09 12:08 PST, Bruno Schmidt
no flags Details
Updated patch with changelog and testcases. (8.32 KB, patch)
2010-04-08 09:45 PDT, Bruno Schmidt
no flags Details | Formatted Diff | Diff
Rebased and changed coding style. (8.32 KB, patch)
2010-04-08 10:51 PDT, Bruno Schmidt
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Fixed the reported patch problems (7.68 KB, patch)
2010-04-09 14:37 PDT, Bruno Schmidt
kenneth: review-
Details | Formatted Diff | Diff
Patch with the corrections requested by Kenneth Rohde Christiansen (7.67 KB, patch)
2010-04-14 14:51 PDT, Bruno Schmidt
kenneth: review-
Details | Formatted Diff | Diff
Patch with the corrections requested by Kenneth (Comment #18) (7.91 KB, patch)
2010-04-14 17:31 PDT, Bruno Schmidt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bruno Schmidt 2010-02-08 15:57:21 PST
Created attachment 48372 [details]
Patch fixing the bug

If an QObject is added to the javascript context and it contains properties of the type QObject* with NULL value, calling the property causes Segmentation Fault.
So now the code checks for objects which are null and threat them accordingly:
 * QtInstance::getClass() may return NULL
 * QtInstance::stringValue(ExecState* exec) may return jsNull()
 * QtInstance::booleanValue() may return false.
Comment 1 Benjamin Poulain 2010-03-05 10:04:03 PST
You need to create an autotest for the bug.

The patches for Webkit requred an entry in the changelog: You execute WebKitTools/Scripts/prepare-changelog --bug 34730, then you edit the changelog to describe the changes, and you include it in the diff.

When submitting the patch, you should set the review and commit queue flag to "?". Otherwise the patch is often ignored.
Comment 2 Bruno Schmidt 2010-03-09 12:08:16 PST
Created attachment 50338 [details]
Bug Test Program

The test program tries to show 3 alerts but the last segfaults because it is a property returning a null QObjetc.
Comment 3 WebKit Review Bot 2010-03-09 12:10:16 PST
Attachment 50338 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Benjamin Poulain 2010-03-09 13:57:49 PST
Comment on attachment 50338 [details]
Bug Test Program

Remove the review flag. This is not a patch, it is a test case.
Comment 5 Benjamin Poulain 2010-03-09 14:00:49 PST
(In reply to comment #2)
> Created an attachment (id=50338) [details]
> Bug Test Program
> 
> The test program tries to show 3 alerts but the last segfaults because it is a
> property returning a null QObjetc.

Actually, I was not asking for a test case but for a unit test in your patch.

We have unittests for QtWebkit in WebKit/qt/tests (those unit test are generally called autotest in Qt because they are run automatically). By making an autotest with your patch, you ensure there will never be a regression for this bug.
Comment 6 Benjamin Poulain 2010-04-06 05:25:48 PDT
Any chance you finish the patch?
Comment 7 Bruno Schmidt 2010-04-08 09:45:50 PDT
Created attachment 52876 [details]
Updated patch with changelog and testcases.

Since I am using git the patch was made usign "git diff" the other steps where as said on "contributing".

Sorry for the delay.
Comment 8 WebKit Review Bot 2010-04-08 09:50:23 PDT
Attachment 52876 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bridge/qt/qt_runtime.cpp:874:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebframe/tst_qwebframe.cpp"
WebCore/bridge/qt/qt_instance.cpp:264:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/bridge/qt/qt_instance.cpp:316:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Benjamin Poulain 2010-04-08 10:27:44 PDT
(In reply to comment #7)
> Created an attachment (id=52876) [details]
> Updated patch with changelog and testcases.
> 
> Since I am using git the patch was made usign "git diff" the other steps where
> as said on "contributing".

The tools works if the patch is done with git, no problem with that.

> Sorry for the delay.

I reset the priority to P1 since you are working on it.

About the coding style, you can check it locally with WebKitTools/Script/check-webkit-style
Comment 10 Bruno Schmidt 2010-04-08 10:51:46 PDT
Created attachment 52877 [details]
Rebased and changed coding style.
Comment 11 Benjamin Poulain 2010-04-08 12:03:14 PDT
Comment on attachment 52877 [details]
Rebased and changed coding style.

Setting the queue flag.
Comment 12 Simon Hausmann 2010-04-09 14:19:23 PDT
Comment on attachment 52877 [details]
Rebased and changed coding style.


> +        No new tests. (OOPS!)

Please remove this line from the ChangeLog. After all you do have unit tests :)

>      // ECMA 9.2
> -    return jsBoolean(true);
> +    return jsBoolean(getObject() ? true : false);

Is the ? true : false part really needed? :)

>              m_readOnlyValue(987),
> +        m_objectStar(0),
>              m_qtFunctionInvoked(-1) { }

The indentation seems off here.

The rest of the patch looks good to me. Good catch, thank you!

r- because of the ChangeLog and indentation.
Comment 13 Bruno Schmidt 2010-04-09 14:37:29 PDT
Created attachment 52989 [details]
Fixed the reported patch problems

Do you know if this patch still can be merged in QtWebKit 2.0?
Comment 14 Benjamin Poulain 2010-04-09 15:20:17 PDT
(In reply to comment #13)
> Do you know if this patch still can be merged in QtWebKit 2.0?

Yep it can.
If the patch is not risky, one can mark it as blocking WebKit 2.0, and Simon will cherry pick it.
Comment 15 Bruno Schmidt 2010-04-13 13:16:51 PDT
(In reply to comment #12)
> (From update of attachment 52877 [details])
> 
> > +        No new tests. (OOPS!)
> 
> Please remove this line from the ChangeLog. After all you do have unit tests :)
> 
> >      // ECMA 9.2
> > -    return jsBoolean(true);
> > +    return jsBoolean(getObject() ? true : false);
> 
> Is the ? true : false part really needed? :)
> 
> >              m_readOnlyValue(987),
> > +        m_objectStar(0),
> >              m_qtFunctionInvoked(-1) { }
> 
> The indentation seems off here.
> 
> The rest of the patch looks good to me. Good catch, thank you!
> 
> r- because of the ChangeLog and indentation.

Those problems are already fixed.
Any news about the merge?
Comment 16 Kenneth Rohde Christiansen 2010-04-14 13:10:51 PDT
Comment on attachment 52989 [details]
Fixed the reported patch problems


> +        QObjects exported to the QWebkit javascript with properties that are a null "QObject*" cause Segmentation Fault.
> +
> +        If an QObject is added to the javascript context and it contains properties of the type QObject* with NULL value, calling the property causes Segmentation Fault.
> +        So now the code checks for objects which are null and threat them accordingly:
> +         * QtInstance::getClass() may return NULL
> +         * QtInstance::stringValue(ExecState* exec) may return jsNull()
> +         * QtInstance::booleanValue() may return false.
> +         * convertQVariantToValue(...) QObjectStar conversion returns jsNull() if obj is null

Can you please wrap the lines at 80-100 chars?

> +
> +        [Qt] Null QObjects properties cause Segmentation Fault
> +        https://bugs.webkit.org/show_bug.cgi?id=34730

This should be at the top.
Comment 17 Bruno Schmidt 2010-04-14 14:51:00 PDT
Created attachment 53370 [details]
Patch with the corrections requested by Kenneth Rohde Christiansen
Comment 18 Kenneth Rohde Christiansen 2010-04-14 15:10:57 PDT
Comment on attachment 53370 [details]
Patch with the corrections requested by Kenneth Rohde Christiansen


>  Class* QtInstance::getClass() const
>  {
> -    if (!m_class)
> +    if (!m_class) {
> +        if (!m_object)
> +            return 0;
>          m_class = QtClass::classForObject(m_object);
> +    }
>      return m_class;
>  }

The code would break other code in the file such as:

MethodList methodList = getClass()->methodsNamed(propertyName, this);

so that might need fixing as well.


> @@ -257,11 +260,14 @@ JSValue QtInstance::defaultValue(ExecState* exec, PreferredPrimitiveType hint) c
>  
>  JSValue QtInstance::stringValue(ExecState* exec) const
>  {
> +    QObject* obj = getObject();
> +    if (!obj)
> +        return jsNull();

Good change!

> +
>      // Hmm.. see if there is a toString defined
>      QByteArray buf;
>      bool useDefault = true;
>      getClass();
> -    QObject* obj = getObject();
>      if (m_class && obj) {

You do not need the obj check here any longer

>  JSValue QtInstance::booleanValue() const
>  {
>      // ECMA 9.2
> -    return jsBoolean(true);
> +    return jsBoolean(getObject());
>  }

Confirmed to be in line with the ECMA 9.2.


> @@ -871,6 +871,8 @@ JSValue convertQVariantToValue(ExecState* exec, PassRefPtr<RootObject> root, con
>  
>      if (type == QMetaType::QObjectStar || type == QMetaType::QWidgetStar) {
>          QObject* obj = variant.value<QObject*>();
> +        if (!obj)
> +            return jsNull();
>          return QtInstance::getQtInstance(obj, root, QScriptEngine::QtOwnership)->createRuntimeObject(exec);
>      }

Awesome!

Good testing!
Comment 19 Bruno Schmidt 2010-04-14 17:27:22 PDT
(In reply to comment #18)

First, thanks for the comments.

I have checked the other implementations and the correct behavior is that the 'Instance' represents an object instance and can't be null.
So nobody checks getClass() result.
The changes I made are only strengthening QtInstance while the real correction is at convertQVariantToValue() by not creating a QtInstance out of a null QObject*.

So I'm changing the code fix the problem reported on QtInstance::getMethod to return jsNull.

> (From update of attachment 53370 [details])
> 
> >  Class* QtInstance::getClass() const
> >  {
> > -    if (!m_class)
> > +    if (!m_class) {
> > +        if (!m_object)
> > +            return 0;
> >          m_class = QtClass::classForObject(m_object);
> > +    }
> >      return m_class;
> >  }
> 
> The code would break other code in the file such as:
> 
> MethodList methodList = getClass()->methodsNamed(propertyName, this);
> 
> so that might need fixing as well.
> 
> 
> > @@ -257,11 +260,14 @@ JSValue QtInstance::defaultValue(ExecState* exec, PreferredPrimitiveType hint) c
> >  
> >  JSValue QtInstance::stringValue(ExecState* exec) const
> >  {
> > +    QObject* obj = getObject();
> > +    if (!obj)
> > +        return jsNull();
> 
> Good change!
> 
> > +
> >      // Hmm.. see if there is a toString defined
> >      QByteArray buf;
> >      bool useDefault = true;
> >      getClass();
> > -    QObject* obj = getObject();
> >      if (m_class && obj) {
> 
> You do not need the obj check here any longer
> 
> >  JSValue QtInstance::booleanValue() const
> >  {
> >      // ECMA 9.2
> > -    return jsBoolean(true);
> > +    return jsBoolean(getObject());
> >  }
> 
> Confirmed to be in line with the ECMA 9.2.
> 
> 
> > @@ -871,6 +871,8 @@ JSValue convertQVariantToValue(ExecState* exec, PassRefPtr<RootObject> root, con
> >  
> >      if (type == QMetaType::QObjectStar || type == QMetaType::QWidgetStar) {
> >          QObject* obj = variant.value<QObject*>();
> > +        if (!obj)
> > +            return jsNull();
> >          return QtInstance::getQtInstance(obj, root, QScriptEngine::QtOwnership)->createRuntimeObject(exec);
> >      }
> 
> Awesome!
> 
> Good testing!
Comment 20 Bruno Schmidt 2010-04-14 17:31:44 PDT
Created attachment 53387 [details]
Patch with the corrections requested by Kenneth (Comment #18)
Comment 21 WebKit Commit Bot 2010-04-15 03:03:39 PDT
Comment on attachment 53387 [details]
Patch with the corrections requested by Kenneth (Comment #18)

Clearing flags on attachment: 53387

Committed r57638: <http://trac.webkit.org/changeset/57638>
Comment 22 WebKit Commit Bot 2010-04-15 03:03:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Simon Hausmann 2010-04-15 14:46:07 PDT
Revision r57638 cherry-picked into qtwebkit-2.0 with commit 1ee9828c1a615345b01e471d34b5efdeb984f13f
Comment 24 Darin Adler 2014-04-24 16:45:10 PDT
Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.