Bug 200954 – REGRESSION (r248807): Objects stored in ElementRareData are leaked

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

@ r248804, the Mojave Leaks bot showed 401 unique leaks
https://build.webkit.org/builders/Apple%20Mojave%20%28Leaks%29/builds/4031

@ r248807, it showed 3519 unique leaks
https://build.webkit.org/builders/Apple%20Mojave%20%28Leaks%29/builds/4032

[Radar WebKit Bug Importer]

<rdar://problem/54536113>

[Radar WebKit Bug Importer]

<rdar://problem/54536121>

[Ryan Haddad]

Looking at the revisions in the range, this looks like a change that could be responsible:

Don't use union to store NodeRareData* and RenderObject*
https://trac.webkit.org/changeset/248807/webkit

[Ryan Haddad]

I can't get the leaks viewer to show any data, it hangs at loading 26/66 files
https://build.webkit.org/LeaksViewer/?url=%2Fresults%2FApple%20Mojave%20%28Leaks%29%2Fr248807%20%284032%29%2F

I see a bunch of 503 errors in the console, but clicking the links for those resources works.

[Ryosuke Niwa]

Created attachment 377181 [details]
Fixes the bug

[David Kilzer (:ddkilzer)]

Comment on attachment 377181 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=377181&action=review

r=me

> Source/WebCore/dom/NodeRareData.h:277
> +    virtual ~NodeRareData()
> +    { }

Do we need to declare this inline?  I recall saving space by not inlining destructors of some classes.  (I guess this is rare, though? :)

[Ryosuke Niwa]

Comment on attachment 377181 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=377181&action=review

>> Source/WebCore/dom/NodeRareData.h:277
>> +    { }
> 
> Do we need to declare this inline?  I recall saving space by not inlining destructors of some classes.  (I guess this is rare, though? :)

Otherwise ElementRareData::~ElementRareData would make an actual function call, which would be silly.
We used to not inline virtual functions to make compilation faster but I don't think it matters for this file.

[Ryosuke Niwa]

Committed r249076: <https://trac.webkit.org/changeset/249076>

[Said Abou-Hallawa]

Comment on attachment 377181 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=377181&action=review

> Source/WebCore/ChangeLog:9
> +        NodeRareData didn't have a virtual destructor. As a result, member variables
> +        of ElementRareData did not get destructed properly.

Usually the complier gives an error/warning if a class is a base class and it does not have a virtual destructor. I wonder what was the reason for having the complier silent in the case of NodeRareData.

[Ryosuke Niwa]

(In reply to Said Abou-Hallawa from comment #9)
> Comment on attachment 377181 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377181&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        NodeRareData didn't have a virtual destructor. As a result, member variables
> > +        of ElementRareData did not get destructed properly.
> 
> Usually the complier gives an error/warning if a class is a base class and
> it does not have a virtual destructor. I wonder what was the reason for
> having the complier silent in the case of NodeRareData.

There is no virtual function in ElementRareData. I wish clang did warn about it too.

Also confirmed that this fixed most of the leak:
https://build.webkit.org/builders/Apple%20Mojave%20%28Leaks%29/builds/4132

[Antti Koivisto]

We shouldn't virtualize a performance sensitive type like this just to be able to destroy it (blot from unnecessary pointer, extra virtual function call, another vtable to corrupt). 

Please revert this patch and implement destruction manually like it was done before.

[Ryosuke Niwa]

Comment on attachment 377181 [details]
Fixes the bug

I don't think it makes sense to mark this patch as r- since it has already been landed.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #11)
> We shouldn't virtualize a performance sensitive type like this just to be
> able to destroy it (blot from unnecessary pointer, extra virtual function
> call, another vtable to corrupt). 

I'm not sure about that. vtable pointer in NodeRareData shouldn't matter because we'd create it for a far fewer DOM nodes after r248807.

> Please revert this patch and implement destruction manually like it was done before.

I've thought about this approach and in fact wrote such a patch locally but decided not to do that because:
1. We won't be able to use unique_ptr or rather defeats the point of it if we had to leakPtr and delete it manually.
2. Relying on Node::m_flags to decide which destructor to use can result in a type confusion if Node::m_flags was heap overrun.

[Ryosuke Niwa]

I think a good alternative is what Saam suggested which is to override deleter, and we can use a single bit of m_connectedFrameCount to record whether it's NodeRareData or not.

This will avoid (1) as well as (2). The flag in NodeRareData being heap overran is a moot point since such an overrun can also override vtable pointer.

[Antti Koivisto]

> I've thought about this approach and in fact wrote such a patch locally but
> decided not to do that because:
> 1. We won't be able to use unique_ptr or rather defeats the point of it if
> we had to leakPtr and delete it manually.

I don't think the aesthetic concern is worth an otherwise unnecessary vptr in this particular code (it may be in other places).

> 2. Relying on Node::m_flags to decide which destructor to use can result in
> a type confusion if Node::m_flags was heap overrun.

Is vptr somehow more secure? This pattern has been used here for a long time.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #15)
> > I've thought about this approach and in fact wrote such a patch locally but
> > decided not to do that because:
> > 1. We won't be able to use unique_ptr or rather defeats the point of it if
> > we had to leakPtr and delete it manually.
> 
> I don't think the aesthetic concern is worth an otherwise unnecessary vptr
> in this particular code (it may be in other places).

It's not an aesthetic concern. It's easy to introduce bugs when exotic memory management mechanisms are used.

> > 2. Relying on Node::m_flags to decide which destructor to use can result in
> > a type confusion if Node::m_flags was heap overrun.
> 
> Is vptr somehow more secure? This pattern has been used here for a long time.

Yes. With vtable pointer, only memory corruption of NodeRareData itself would result in  type confusion whereas the old code would have let Node::m_nodeFlags to cause a type confusion in NodeRareData itself. Since the whole point of r248807 is not let that kind of type confusion happen (which had been a source of number of security bugs), going back to the old model isn't desirable.

[Ryosuke Niwa]

Let's go with the deleter approach.

[Antti Koivisto]

(In reply to Ryosuke Niwa from comment #14)
> I think a good alternative is what Saam suggested which is to override
> deleter, and we can use a single bit of m_connectedFrameCount to record
> whether it's NodeRareData or not.
> 
> This will avoid (1) as well as (2). The flag in NodeRareData being heap
> overran is a moot point since such an overrun can also override vtable
> pointer.

That seems like a good plan. 

Clearly my review of r248807 was too hasty, it shouldn't have been r+'d in the first place considering this obvious issue. Sorry about that.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #18)
> (In reply to Ryosuke Niwa from comment #14)
> > I think a good alternative is what Saam suggested which is to override
> > deleter, and we can use a single bit of m_connectedFrameCount to record
> > whether it's NodeRareData or not.
> > 
> > This will avoid (1) as well as (2). The flag in NodeRareData being heap
> > overran is a moot point since such an overrun can also override vtable
> > pointer.
> 
> That seems like a good plan. 
> 
> Clearly my review of r248807 was too hasty, it shouldn't have been r+'d in
> the first place considering this obvious issue. Sorry about that.

Wanna use a new bug to override deleter? I was thinking that I can just re-open this bug and post a new patch.

[Antti Koivisto]

Re-opening is good.

[Ryosuke Niwa]

Reopening.

[Ryosuke Niwa]

Ugh... I have to fix makeUnique because it doesn't know how to take Deleter or default_delete. Probably have to do that on Monday.

[Ryosuke Niwa]

Created attachment 377423 [details]
Address Antti's comment

[Antti Koivisto]

Comment on attachment 377423 [details]
Address Antti's comment

This is a good way to do this.

[Ryosuke Niwa]

Comment on attachment 377423 [details]
Address Antti's comment

Clearing flags on attachment: 377423

Committed r249198: <https://trac.webkit.org/changeset/249198>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.