Bug 25930 – Mismatched free

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[xxx]

valgrind reports free is called where delete[] should be called.
Not sure if bug is at CSS or WTF

WebKit-r43887/WebKit/qt/tests/qwebpage> valgrind --track-origins=yes ./tst_qwebpage

=8572== Mismatched free() / delete / delete []
==8572==    at 0x4023E5A: free (vg_replace_malloc.c:323)
==8572==    by 0x42FD0D5: WTF::fastFree(void*) (FastMalloc.cpp:216)
==8572==    by 0x43DA8B8: WebCore::CSSSelectorList::adoptSelectorVector(WTF::Vector<WebCore::CSSSelector*, 0u>&) (CSSSelectorList.cpp:57)
==8572==    by 0x43C4F8E: WebCore::CSSStyleRule::adoptSelectorVector(WTF::Vector<WebCore::CSSSelector*, 0u>&) (CSSStyleRule.h:53)
==8572==    by 0x43C213E: WebCore::CSSParser::createStyleRule(WTF::Vector<WebCore::CSSSelector*, 0u>*) (CSSParser.cpp:4547)
==8572==    by 0x4C85EEF: cssyyparse(void*) (CSSGrammar.y:790)
==8572==    by 0x43ACAAB: WebCore::CSSParser::parseSheet(WebCore::CSSStyleSheet*, WebCore::String const&) (CSSParser.cpp:224)
==8572==    by 0x441761E: WebCore::CSSStyleSheet::parseString(WebCore::String const&, bool) (CSSStyleSheet.cpp:164)
==8572==    by 0x43DFE14: WebCore::parseUASheet(WebCore::String const&) (CSSStyleSelector.cpp:489)
==8572==    by 0x43DFE51: WebCore::parseUASheet(char const*, unsigned int) (CSSStyleSelector.cpp:495)
==8572==    by 0x43E02EB: WebCore::loadSimpleDefaultStyle() (CSSStyleSelector.cpp:534)
==8572==    by 0x43DEEEB: WebCore::CSSStyleSelector::CSSStyleSelector(WebCore::Document*, WebCore::String const&, WebCore::StyleSheetList*, WebCore::CSSStyleSheet*, bool, bool) (CSSStyleSelector.cpp:409)
==8572==    by 0x4439BEF: WebCore::Document::attach() (Document.cpp:1271)
==8572==    by 0x4689111: WebCore::Frame::setDocument(WTF::PassRefPtr<WebCore::Document>) (Frame.cpp:245)
==8572==    by 0x4606CA7: WebCore::FrameLoader::begin(WebCore::KURL const&, bool, WebCore::SecurityOrigin*) (FrameLoader.cpp:938)
==8572==  Address 0x6c8c348 is 0 bytes inside a block of size 16 alloc'd
==8572==    at 0x4024A4E: operator new(unsigned int) (vg_replace_malloc.c:224)
==8572==    by 0x43C1333: WebCore::CSSParser::createFloatingSelector() (CSSParser.cpp:4382)
==8572==    by 0x4C86343: cssyyparse(void*) (CSSGrammar.y:881)
==8572==    by 0x43ACAAB: WebCore::CSSParser::parseSheet(WebCore::CSSStyleSheet*, WebCore::String const&) (CSSParser.cpp:224)
==8572==    by 0x441761E: WebCore::CSSStyleSheet::parseString(WebCore::String const&, bool) (CSSStyleSheet.cpp:164)
==8572==    by 0x43DFE14: WebCore::parseUASheet(WebCore::String const&) (CSSStyleSelector.cpp:489)
==8572==    by 0x43DFE51: WebCore::parseUASheet(char const*, unsigned int) (CSSStyleSelector.cpp:495)
==8572==    by 0x43E02EB: WebCore::loadSimpleDefaultStyle() (CSSStyleSelector.cpp:534)
==8572==    by 0x43DEEEB: WebCore::CSSStyleSelector::CSSStyleSelector(WebCore::Document*, WebCore::String const&, WebCore::StyleSheetList*, WebCore::CSSStyleSheet*, bool, bool) (CSSStyleSelector.cpp:409)
==8572==    by 0x4439BEF: WebCore::Document::attach() (Document.cpp:1271)
==8572==    by 0x4689111: WebCore::Frame::setDocument(WTF::PassRefPtr<WebCore::Document>) (Frame.cpp:245)
==8572==    by 0x4606CA7: WebCore::FrameLoader::begin(WebCore::KURL const&, bool, WebCore::SecurityOrigin*) (FrameLoader.cpp:938)
==8572==    by 0x4603631: WebCore::FrameLoader::init() (FrameLoader.cpp:292)
==8572==    by 0x4688E02: WebCore::Frame::init() (Frame.cpp:189)
==8572==    by 0x486B5A7: QWebFramePrivate::init(QWebFrame*, WebCore::Page*, QWebFrameData*) (qwebframe.cpp:130)

[Darin Adler]

*** Bug 27211 has been marked as a duplicate of this bug. ***

[Zoltan Horvath]

Created attachment 38980 [details]
proposed patch

What is your opinion about this solution?

[xxx]

(In reply to comment #2)
> Created an attachment (id=38980) [details]
> proposed patch
> 
> What is your opinion about this solution?

Not sure the question is addressed to me, since I'm not acquanted with the project, but reviewing the implementation of fastNew/fastDelte, it sounds good.

[Darin Adler]

Comment on attachment 38980 [details]
proposed patch

This may quiet valgrind, but it doesn't solve the problem.

There's still a mismatch. The allocation is fastNew of a CSSSelector and the deallocation is fastDelete of a char. The fact that this currently works doesn't mean it's matched up.

This also introduces a new mismatch, between the fastNew in CSSParser::createFloatingSelector() and the delete done by the call to deleteAllValues in ~CSSParser. If we change to fastNew we need to change the delete to fastDelete.

The usage should be just fastDelete, not WTF::fastDelete.

---

We could add a variant of fastDelete that does not call a destructor, and fix the mismatch that way. There may be some other good solution.

[Zoltan Horvath]

Created attachment 39346 [details]
updated proposed patch

[Darin Adler]

*** Bug 28726 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 39346 [details]
updated proposed patch

The name fastPureDelete isn't so clear. I think fastDeleteSkippingDestructor would be better.

I think that "pure" is not a good term because it makes it sound like the other delete is not as good as this one. It's this strange deletion without destruction that needs the longer, uglier names.

> +    template <typename T>
> +    inline void fastPureDelete(T* p)

> -// Using WTF::FastAllocBase to avoid using FastAllocBase's explicit qualification by WTF::.
> +// Avoid explicit qualification by WTF::

I don't think there should be a comment here at all. The other WTF files don't have a comment before their using statements.

> +    template<typename ValueType, typename HashTableType>
> +    void fastDeleteAllValues(HashTableType& collection)
> +    {
> +        typedef typename HashTableType::const_iterator iterator;
> +        iterator end = collection.end();
> +        for (iterator it = collection.begin(); it != end; ++it)
> +            fastDelete(*it);
> +    }
> +
> +    template<typename T, typename U, typename V>
> +    inline void fastDeleteAllValues(const HashSet<T, U, V>& collection)
> +    {
> +        fastDeleteAllValues<typename HashSet<T, U, V>::ValueType>(collection.m_impl);
> +    }

It's very sad to have two versions of this because we have two versions of delete. In the end it's not scalable to have two of everything related to new and delete.

r=me because my only quibble is with a function name

[Darin Adler]

Comment on attachment 39346 [details]
updated proposed patch

> Index: JavaScriptCore/wtf/HashSet.h
> ===================================================================
> --- JavaScriptCore/wtf/HashSet.h	(revision 47860)
> +++ JavaScriptCore/wtf/HashSet.h	(working copy)
> @@ -91,7 +91,7 @@ namespace WTF {
>  
>      private:
>          friend void deleteAllValues<>(const HashSet&);
> -
> +    public:
>          HashTableType m_impl;
>      };

Wait, this isn't right!? m_impl should not be public! The fastDeleteAllValues function needs to be a friend just as deleteAllValues is.

[Yong Li]

*** Bug 29116 has been marked as a duplicate of this bug. ***

[Yong Li]

When USE_SYSTEM_ALLOC=1 (or USE_FAST_ALLOC_BASE=0), we should disable FastAllocBase.

namespace WTF {

    class FastAllocBase {
    };

    template <typename T>
    inline T* fastNew() { return new T; }

    template <typename T, typename Arg1>
    inline T* fastNew(Arg1 arg1) { return new T(arg1); }

    template <typename T, typename Arg1, typename Arg2>
    inline T* fastNew(Arg1 arg1, Arg2 arg2) { return new T(arg1, arg2); }

    template <typename T, typename Arg1, typename Arg2, typename Arg3>
    inline T* fastNew(Arg1 arg1, Arg2 arg2, Arg3 arg3) { return new T(arg1, arg2, arg3); }

    template <typename T, typename Arg1, typename Arg2, typename Arg3, typename Arg4>
    inline T* fastNew(Arg1 arg1, Arg2 arg2, Arg3 arg3, Arg4 arg4) { return new T(arg1, arg2, arg3, arg4); }

    template <typename T, typename Arg1, typename Arg2, typename Arg3, typename Arg4, typename Arg5>
    inline T* fastNew(Arg1 arg1, Arg2 arg2, Arg3 arg3, Arg4 arg4, Arg5 arg5) { return new T(arg1, arg2, arg3, arg4, arg5); }

    template <typename T>
    inline T* fastNewArray(size_t count) { return new T[count]; }

    template <typename T>
    inline void fastDelete(T* p) { delete p; }

    template <typename T>
    inline void fastDeleteArray(T* p) { delete[] p; }

    template <typename T>
    inline void fastNonNullDelete(T* p) { delete p; }

    template <typename T>
    inline void fastNonNullDeleteArray(T* p) { delete[] p; }

} // namespace WTF

[Zoltan Horvath]

Created attachment 39352 [details]
updated proposed patch v2

[Darin Adler]

Comment on attachment 39352 [details]
updated proposed patch v2

> -        delete reinterpret_cast<char*>(selectorVector[i]);
> +        fastDeleteSkippingDestructor(reinterpret_cast<char*>(selectorVector[i]));

This is wrong. There is no need for the reinterpret_cast any more. Also the comment needs to be updated.

[Zoltan Horvath]

Created attachment 39353 [details]
updated proposed file v3

Darin: I did all modifications that you mentioned. Sorry for the mistakes...

Yong Li: I think we shouldn't disable FastAllocBase in the case of USE_SYSTEM_MALLOC=1, because that should work with USE_SYSTEM_MALLOC=1 as well.

[Darin Adler]

Comment on attachment 39353 [details]
updated proposed file v3

> -        // We want to free the memory (which was allocated with new), but we
> +        // We want to free the memory (which was allocated with fastNew), but we
>          // don't want the destructor to run since it will affect the copy
> -        // we've just made. In theory this is undefined, but operator delete
> -        // is only defined taking a void*, so in practice it should be ok.
> -        delete reinterpret_cast<char*>(selectorVector[i]);
> +        // we've just made. We use fastDeleteSkippingDestructor function, because
> +        // that is releasing without destructor call.
> +        fastDeleteSkippingDestructor(selectorVector[i]);

I think we should leave out the last sentence of the comment here. The name of the function is clear enough and matches the comment in an obvious way.

[Zoltan Horvath]

I did the comment change.

Landed in @48259. http://trac.webkit.org/changeset/48259