Bug 122277 – Missing NULL check at destruct API of VectorDestructor

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Template Framework

[Byungseon(Sun) Shin]

When gcc auto-vectorizer option enabled, arguments of destruct API are null.
So it make WebProcess got crashed.

[Byungseon Shin]

Created attachment 213270 [details]
Patch

[Anders Carlsson]

Comment on attachment 213270 [details]
Patch

I don't think this is correct. Which of begin and end are null?

[Byungseon(Sun) Shin]

both variables are null.

[Anders Carlsson]

(In reply to comment #3)
> both variables are null.

If both begin and end are null the loop condition will be false and the loop body will not be entered - see http://en.wikipedia.org/wiki/C_syntax#Iteration_statements for more information.

Maybe the compiler is buggy?

[Byungseon Shin]

Created attachment 213311 [details]
Patch

[Byungseon(Sun) Shin]

I have checked it again and found that only begin variable is null on ARM-linux platform with auto-vectorization.
So, it seems platform specific bug.

[Anders Carlsson]

(In reply to comment #6)
> I have checked it again and found that only begin variable is null on ARM-linux platform with auto-vectorization.
> So, it seems platform specific bug.

I don't think we can work around the bug in this manner. What's the vector that ends up being destroyed with a null begin?

[Byungseon(Sun) Shin]

@andersca, I agree with your idea.
Here are some info which I got it from Valgrind.

==7097== Invalid read of size 4
==7097==    at 0x4CD8704: WebCore::DocumentStyleSheetCollection::updateActiveStyleSheets(WebCore::DocumentStyleSheetCollection::UpdateFlag) (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x57B72B5: WebCore::Document::styleResolverChanged(WebCore::StyleResolverUpdateFlag) (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x57B73BF: WebCore::Document::didRemoveAllPendingStylesheet() (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x5635ADD: WebCore::HTMLLinkElement::sheetLoaded() (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x4CCF9CB: WebCore::StyleSheetContents::checkLoaded() (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x564ACF5: WebCore::HTMLLinkElement::setCSSStyleSheet(WTF::String const&, WebCore::KURL const&, WTF::String const&, WebCore::CachedCSSStyleSheet const*) (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x4D5DBF3: WebCore::CachedCSSStyleSheet::checkNotify() (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x4D5D8D3: WebCore::CachedCSSStyleSheet::finishLoading(WebCore::ResourceBuffer*) (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x4D9FB73: WebCore::SubresourceLoader::didFinishLoading(double) (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x4D98EDB: WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x4EED8FF: WebCore::QNetworkReplyHandler::finish() (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==    by 0x4EED71F: WebCore::QNetworkReplyHandlerCallQueue::flush() (in /usr/lib/libQt5WebKit.so.5.0.0)
==7097==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==7097==

[Byungseon(Sun) Shin]

It seems like this called when activeStyleSheets vector destroys while DocumentStyleSheetCollection::updateActiveStyleSheets return.

[Anders Carlsson]

(In reply to comment #9)
> It seems like this called when activeStyleSheets vector destroys while DocumentStyleSheetCollection::updateActiveStyleSheets return.

The activeStyleSheets vector has its contents swapped out with m_styleSheetsForStyleSheetList. You could try to figure out what happens in the call to Vector::swap.

Also, which version of GCC are you building with?

[Byungseon(Sun) Shin]

I have tested both version and has same result
1.gcc version 4.6.4 20120731 (prerelease) (crosstool-NG 1.15.3 - 2012.11_nc4)
2. gcc version 4.7.4 20130626 (prerelease) (crosstool-NG linaro-1.13.1-4.7-2013.04-20130415 - 4.7-2013.08-rc4)

[Byungseon Shin]

Created attachment 213501 [details]
Patch

[Byungseon(Sun) Shin]

Even after m_styleSheetsForStyleSheetList.swap(activeStyleSheets), activeStyleSheets size value preserved when updateActiveStyleSheets returns.
However, when destructor tries to delete elements, it had been cleared by swap call. That's why crash happens.

I couldn't understand why swap call needed because both local variables could be destroyed when it returns.
So, I have modified a patch to do copy of each vectors.
And now crash is gone.

[Anders Carlsson]

(In reply to comment #13)
> Even after m_styleSheetsForStyleSheetList.swap(activeStyleSheets), activeStyleSheets size value preserved when updateActiveStyleSheets returns.
> However, when destructor tries to delete elements, it had been cleared by swap call. That's why crash happens.
> 
> I couldn't understand why swap call needed because both local variables could be destroyed when it returns.

Swap is used for performance reasons since copying a vector is expensive. 

If you look at http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Vector.h#L667 you do see that swap correctly swaps m_size so this has got to be a compiler bug. I suggest you build with vectorization turned off.

[Byungseon(Sun) Shin]

Thanks for the comments.
I will discuss this issue with compiler engineer and get back to it.

[Ahmad Saleem]

By looking into the patch, I noticed that the 'DocumentStyleSheetCollection.cpp' was removed by following commi -https://github.com/WebKit/WebKit/commit/2c152c72ab20e97cffb6f1e5d3d2397c139ea0a4

Although the line of goal, which was modified still exists in below file:

https://github.com/WebKit/WebKit/blob/a39b520b8e0ca63489db7a45fe19181a33fac903/Source/WebCore/style/StyleScope.cpp#L517

Appreciate if someone can look into to confirm whether something further is required? Thanks!

[Ryosuke Niwa]

This is won't fix at this point.