Bug 122277

Summary: Missing NULL check at destruct API of VectorDestructor
Product: WebKit Reporter: Byungseon Shin <sun.shin>
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Major CC: andersca, benjamin, cmarcelo, commit-queue, esprehn+autocc, joone, kangil.han, mark.lam, simon.pena, sun.shin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch andersca: review-

Description Byungseon Shin 2013-10-03 09:02:32 PDT
When gcc auto-vectorizer option enabled, arguments of destruct API are null.
So it make WebProcess got crashed.
Comment 1 Byungseon Shin 2013-10-03 09:34:02 PDT
Created attachment 213270 [details]
Patch
Comment 2 Anders Carlsson 2013-10-03 09:57:04 PDT
Comment on attachment 213270 [details]
Patch

I don't think this is correct. Which of begin and end are null?
Comment 3 Byungseon Shin 2013-10-03 10:29:25 PDT
both variables are null.
Comment 4 Anders Carlsson 2013-10-03 11:33:39 PDT
(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?
Comment 5 Byungseon Shin 2013-10-03 16:48:10 PDT
Created attachment 213311 [details]
Patch
Comment 6 Byungseon Shin 2013-10-03 16:50:32 PDT
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.
Comment 7 Anders Carlsson 2013-10-03 17:33:38 PDT
(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?
Comment 8 Byungseon Shin 2013-10-03 18:16:10 PDT
@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==
Comment 9 Byungseon Shin 2013-10-03 19:14:27 PDT
It seems like this called when activeStyleSheets vector destroys while DocumentStyleSheetCollection::updateActiveStyleSheets return.
Comment 10 Anders Carlsson 2013-10-03 20:40:52 PDT
(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?
Comment 11 Byungseon Shin 2013-10-03 21:23:42 PDT
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)
Comment 12 Byungseon Shin 2013-10-06 00:29:20 PDT
Created attachment 213501 [details]
Patch
Comment 13 Byungseon Shin 2013-10-06 00:38:12 PDT
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.
Comment 14 Anders Carlsson 2013-10-06 08:30:39 PDT
(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.
Comment 15 Byungseon Shin 2013-10-06 11:30:15 PDT
Thanks for the comments.
I will discuss this issue with compiler engineer and get back to it.