Bug 34730 – [Qt] Null QObjects properties cause Segmentation Fault

• Component: JavaScriptCore
• Status: CLOSED FIXED

[Bruno Schmidt]

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.

[Benjamin Poulain]

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.

[Bruno Schmidt]

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.

[WebKit Review Bot]

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.

[Benjamin Poulain]

Comment on attachment 50338 [details]
Bug Test Program

Remove the review flag. This is not a patch, it is a test case.

[Benjamin Poulain]

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

[Benjamin Poulain]

Any chance you finish the patch?

[Bruno Schmidt]

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.

[WebKit Review Bot]

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.

[Benjamin Poulain]

(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

[Bruno Schmidt]

Created attachment 52877 [details]
Rebased and changed coding style.

[Benjamin Poulain]

Comment on attachment 52877 [details]
Rebased and changed coding style.

Setting the queue flag.

[Simon Hausmann]

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.

[Bruno Schmidt]

Created attachment 52989 [details]
Fixed the reported patch problems

Do you know if this patch still can be merged in QtWebKit 2.0?

[Benjamin Poulain]

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

[Bruno Schmidt]

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

[Kenneth Rohde Christiansen]

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.

[Bruno Schmidt]

Created attachment 53370 [details]
Patch with the corrections requested by Kenneth Rohde Christiansen

[Kenneth Rohde Christiansen]

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!

[Bruno Schmidt]

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

[Bruno Schmidt]

Created attachment 53387 [details]
Patch with the corrections requested by Kenneth (Comment #18)

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r57638 cherry-picked into qtwebkit-2.0 with commit 1ee9828c1a615345b01e471d34b5efdeb984f13f

[Darin Adler]

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.