Bug 22834 – Mismatched memory free in the new CSSSelectorList

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: CSS

[Brett Wilson (Google)]

CSSParser::createFloatingSelector makes new CSSSelectors using new. These are passed around and eventually put into a CSSSelectorList. This list uses fastFree to free the data in the for loop in CSSSelectorList::adoptSelectorVector. I think the free should instead be delete.

[Brett Wilson (Google)]

Created attachment 25992 [details]
Patch

I'm not super familiar with this code, so I'm not sure if there are other ways for the selectors to be created. Please check carefully.

[Darin Adler]

Comment on attachment 25992 [details]
Patch

r=me

[Brett Wilson (Google)]

Comment on attachment 25992 [details]
Patch

I now think this patch is wrong. I'll try to come up with another solution.

[Brett Wilson (Google)]

Created attachment 26026 [details]
Patch v2

I believe this patch is correct. The previous patch caused double delete problems, which only appeared on Windows for some reason. The problem is that delete also calls the destructor, so when the selector is "moved" in CSSSelectorList, it calls the destructor, then the destructor is called again when the list is freed.

This patch uses fastMalloc for the allocation to match the move and delete operations. This is my first ever use of placement new, so check my syntax carefully :)

I added a few comments about the allocation method since certain assumptions are made which aren't obvious (causing bugs like this one).

[Eric Seidel (no email)]

I r-'d the old patch now that brett says it's wrong (to remove it from the commit queue)

[Darin Adler]

Comment on attachment 26026 [details]
Patch v2

Sadly, this is still wrong, but now it's wrong for the case where parsing failed.

In ~CSSParser(), deleteAllValues(m_floatingSelectors) is called, which will call delete on any selectors that are still in the m_floatingSelectors HashSet. So we can't allocate them in a way where delete won't work.

Maybe the best design is to add a function to CSSSelector that causes it to drop its data, and call it before calling delete in CSSSelectorList::adoptSelectorVector. It would be enough to just clear out the m_data union.

[Antti Koivisto]

We have global operator new which does fastMalloc(). It should be safe to fastFree() objects.

[Antti Koivisto]

Finding better hackery to make purify happy would be good, but not in expense of loosing optimizations.

[Brett Wilson (Google)]

I think Antti is correct and I don't have any brilliant ideas (after discussing with him a bit on IRC) for improving this code without hurting performance.

[Dean McNamee]

(In reply to comment #7)
> We have global operator new which does fastMalloc(). It should be safe to
> fastFree() objects.
> 

I don't think this should be relied upon.  The global operator new is optional (USE_SYSTEM_MALLOC), and specifically it's important not to rely on an application global new override, when embedders may want to opt out of that, or supply their own.  For example, Chromium Linux current doesn't work w/o USE_SYSTEM_MALLOC, because of complicated allocator mismatches (Paul Pedriana seems to be working on this problem).  It would be nice if we could override our new / delete to use a different allocator, but current that would mean that when you use new you get one allocator, and when you use fastFree (which will then use free()), you would get another allocator.  It would be allow that malloc/free and new/delete use different incompatible allocators.

In general I think new / delete / free mismatches can be a lot of trouble, and it's really important to keep them consistent, our you're really limiting the allocation options for embedders.  It's more than just pacifying purify, and we're already our Linux port is already seeing problems with the WebKit allocation design.

[Darin Adler]

(In reply to comment #10)
> (In reply to comment #7)
> > We have global operator new which does fastMalloc(). It should be safe to
> > fastFree() objects.
> 
> I don't think this should be relied upon.

I agree.

[Dean McNamee]

Created attachment 26766 [details]
Patch attempt.

This is really not the cleanest approach, and technically by the C++ standard the behavior is undefined.  However, I think it's an improvement over the current allocation mismatch.

[Darin Adler]

Comment on attachment 26766 [details]
Patch attempt.

I don't think this is enough of an improvement.

If we're going to go to this trouble I wish we could do it right. The best way I can think of would be to implement swap for CSSSelector and use swap and delete instead of memcpy and fastFree.

[Dean McNamee]

Another hacky and probably inappropriate solution would be to zero the old selector after we call memcpy on it, and before calling delete on it.  We are still going to need the second half of the patch to control whether we use fastMalloc/fastFree, or delete.  However, with the way it's written, it seems like it will be a bit awkward.  Any thoughts on zeroing?

I will work on the swap implementation.

(In reply to comment #13)
> (From update of attachment 26766 [details] [review])
> I don't think this is enough of an improvement.
> 
> If we're going to go to this trouble I wish we could do it right. The best way
> I can think of would be to implement swap for CSSSelector and use swap and
> delete instead of memcpy and fastFree.
> 

[Brett Wilson (Google)]

I personally like the swap approach. I use it in my code from time to time in similar circumstances.

[Dean McNamee]

Well, we will be swapping uninitialized memory into the old Selector, and then it's destructor is going to crash.  You can't really implement a swap, you would need to implement a copyToAndClearSelf(CSSSelector otherSelector).  It seems a bit awkward to me.

The other option is to actually call the constructor, and use new[] instead of fastMalloc.  But I thought the whole argument originally was that there was some performance at stake, and now it would seem like we're doing a lot more work.

Let me know what you think is the best option.  Thanks.

(In reply to comment #15)
> I personally like the swap approach. I use it in my code from time to time in
> similar circumstances.
> 

[Dean McNamee]

Hi,

My patch to use a delete reinterpret_cast<> is effectively the same thing as what's happening now, but it also fixes a real bug.  I feel like this is one of those cases where the patch isn't perfect, so instead we'll do nothing.  If someone is more familiar with the code and performance requirements, and is going to make a cleaner fix, that would be great.  In the meantime my patch would at least fix a bug.

(In reply to comment #16)
> Well, we will be swapping uninitialized memory into the old Selector, and then
> it's destructor is going to crash.  You can't really implement a swap, you
> would need to implement a copyToAndClearSelf(CSSSelector otherSelector).  It
> seems a bit awkward to me.
> 
> The other option is to actually call the constructor, and use new[] instead of
> fastMalloc.  But I thought the whole argument originally was that there was
> some performance at stake, and now it would seem like we're doing a lot more
> work.
> 
> Let me know what you think is the best option.  Thanks.
> 
> (In reply to comment #15)
> > I personally like the swap approach. I use it in my code from time to time in
> > similar circumstances.
> > 
> 

[Tony Chang]

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

[Darin Adler]

Comment on attachment 26766 [details]
Patch attempt.

> -        fastFree(selectorVector[i]);
> +        // We want to free the memory (which was allocated with new), 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]);

While this is no better than fastFree in practice, you've convinced me it’s worthwhile because it makes valgrind stop complaining, and this makes it slightly more likely the code would work if we made delete and fastFree incompatible with each other.

Once this is done, we still have a bug, but I suppose it’s a less serious one than what we have now.

> +    // We had two cases in adoptSelectVector.  The fast case of a 1 element
> +    // vector took the CSSSelector directly, which was allocated with new.
> +    // The second case we allocated a new fastMalloc buffer, which sholud be
> +    // freed with fastFree, and the destructors called manually.

Whoever lands this should fix the typo in "sholud".

We also normally do one space after a period, not two.

r=me

[David Levin]

Assigned to levin for landing.

[David Levin]

Please add a link to the bug in the changelog in the future.

[David Levin]

Committed as r41877.

[Zoltan Horvath]

FastMalloc mismatch validation system (Platform.h, FAST_FAST_MALLOC_MATCH_VALIDATION=1) crashes on Linux-Qt in CSSSelectorList due to mismatch.  
Benbuck Nason has a solution for the problem, I reopen the bug.

[Benbuck Nason]

Created attachment 46375 [details]
Patch for comment #23

[WebKit Review Bot]

Attachment 46375 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6

[Benbuck Nason]

Created attachment 46376 [details]
Patch for comment #23 (retry)

[Darin Adler]

Comment on attachment 46376 [details]
Patch for comment #23 (retry)

The right fix is to change CSSParser.cpp to call "new CSSSelector" instead of "fastNew<CSSSelector>()".

> -                delete m_data.m_tagHistory;
> +                fastDelete(m_data.m_tagHistory);

This isn't right. When m_hasRareData is true, then the tag history will be deleted as part of the destruction of OwnPtr. This will call delete, not fastDelete. So changing the code in ~CSSSelector to call fastDelete will not fix the mismatch. But since CSSSelector inherits from Noncopyable, there is no reason to explicitly use fastNew on a CSSSelector -- Noncopyable inherits from FastAllocBase, and so new should automatically use fastNew. This code change seems both incorrect and unnecessary.

> -        delete s;
> +        fastDelete(s);

Same comment here. Changing these call sites is incompatible with using OwnPtr<CSSSelector> elsewhere.

[Zoltan Horvath]

(In reply to comment #27)
> (From update of attachment 46376 [details])
> The right fix is to change CSSParser.cpp to call "new CSSSelector" instead of
> "fastNew<CSSSelector>()".

We put fastNew into CSSParser.cpp to avoid valgrind's mismatched warnings in bug #25930.

[Eric Seidel (no email)]

Comment on attachment 26766 [details]
Patch attempt.

Clearing Darin Adler's review on this old patch and marking obsolete.  This patch was already landed by Levin, see above.

[Eric Seidel (no email)]

Comment on attachment 26026 [details]
Patch v2

Marking this old patch obsolete now that this bug has been re-opened for a second change.

[David Levin]

Cleared myself from being the assignee (since I only did this for landing the patch).

[Brent Fulgham]

This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.