Oilpan: blink_heap_unittests: asan check failed. HeapObjectHeader::finalize zap a wrong memory for …

Oilpan: blink_heap_unittests: asan check failed. HeapObjectHeader::finalize zap a wrong memory for object without VTable

1. In the HeapObjectHeader::finalize, we zap the primary vTable entry, but we have not checked if the object has vTable.

R=ager@chromium.org, oilpan-reviews@chromium.org
BUG=411204
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181596

[haraken, 2014-09-06 06:46:12]

Just help me understand: Why is it OK to zap the primary vtable, but not OK to
zap the first address of the object if the object does not have a vtable?

[Daniel Chow, 2014-09-06 07:36:54]

On 2014/09/06 06:46:12, haraken wrote:
> Just help me understand: Why is it OK to zap the primary vtable, but not OK to
> zap the first address of the object if the object does not have a vtable?

On 64-bit systems, zappedVTable is a intptr_t with size of 8, 
and if the object size less then 8, the zap operation will set memory outside of
the bounds of this object.
When a class have no VTable, its instance size may be less then 8, such as
http://code.google.com/p/chromium/issues/detail?id=411204

[haraken, 2014-09-06 07:51:10]

On 2014/09/06 07:36:54, Daniel Chow wrote:
> On 2014/09/06 06:46:12, haraken wrote:
> > Just help me understand: Why is it OK to zap the primary vtable, but not OK
to
> > zap the first address of the object if the object does not have a vtable?
> 
> On 64-bit systems, zappedVTable is a intptr_t with size of 8, 
> and if the object size less then 8, the zap operation will set memory outside
of
> the bounds of this object.
> When a class have no VTable, its instance size may be less then 8, such as
> http://code.google.com/p/chromium/issues/detail?id=411204

Thanks, that makes a lot of sense! LGTM.

[Daniel Chow, 2014-09-06 08:30:35]

The CQ bit was checked by zhishun.zhou@samsung.com

[commit-bot: I haz the power, 2014-09-06 08:31:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhishun.zhou@samsung.com/547903002/1

[commit-bot: I haz the power, 2014-09-06 10:34:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-06 10:34:47]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25868)

[zerny-chromium, 2014-09-08 05:44:25]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-09-08 05:45:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhishun.zhou@samsung.com/547903002/1

[zerny-chromium, 2014-09-08 05:45:46]

Thanks and lgtm. Checking CQ again after unrelated win failures.

[commit-bot: I haz the power, 2014-09-08 07:34:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-08 07:34:29]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25975)

[Mads Ager (chromium), 2014-09-08 08:59:46]

I'm fine with landing this change. However, I would like to understand better
what is going on. We always round up allocations to the allocation granularity
which is 8-bytes. Therefore, we should never have objects allocated that are
less than 8 bytes in size. I'll see if I can reproduce this and inspect the
object for which this is happening.

[Daniel Chow, 2014-09-09 02:27:42]

The CQ bit was checked by zhishun.zhou@samsung.com

[commit-bot: I haz the power, 2014-09-09 02:27:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhishun.zhou@samsung.com/547903002/1

[commit-bot: I haz the power, 2014-09-09 03:01:43]

Committed patchset #1 (id:1) as 181596

[Daniel Chow, 2014-09-09 03:33:31]

On 2014/09/08 08:59:46, Mads Ager (chromium) wrote:
> I'm fine with landing this change. However, I would like to understand better
> what is going on. We always round up allocations to the allocation granularity
> which is 8-bytes. Therefore, we should never have objects allocated that are
> less than 8 bytes in size. I'll see if I can reproduce this and inspect the
> object for which this is happening.

it's happened in this situation: 
our 8-bytes allocation granularity is for (size of object + size of Header).  
when size of object is 4, and size of HeapObjectHeader is 4, the sum meets the
8-bytes allocation granularity.
for Release version on my 64-bit systems, size of BasicObjectHeader is 4 and
size of HeapObjectHeader is 4, because  sizeof(unsigned) is 4:
in class PLATFORM_EXPORT BasicObjectHeader :
protected:
    volatile unsigned m_size;   // ----> now it's 4 bytes; in old version we use
size_t, that's 8-bytes.

[Mads Ager (chromium), 2014-09-09 07:41:27]

On 2014/09/09 03:33:31, Daniel Chow wrote:
> On 2014/09/08 08:59:46, Mads Ager (chromium) wrote:
> > I'm fine with landing this change. However, I would like to understand
better
> > what is going on. We always round up allocations to the allocation
granularity
> > which is 8-bytes. Therefore, we should never have objects allocated that are
> > less than 8 bytes in size. I'll see if I can reproduce this and inspect the
> > object for which this is happening.
> 
> it's happened in this situation: 
> our 8-bytes allocation granularity is for (size of object + size of Header).  
> when size of object is 4, and size of HeapObjectHeader is 4, the sum meets the
> 8-bytes allocation granularity.
> for Release version on my 64-bit systems, size of BasicObjectHeader is 4 and
> size of HeapObjectHeader is 4, because  sizeof(unsigned) is 4:
> in class PLATFORM_EXPORT BasicObjectHeader :
> protected:
>     volatile unsigned m_size;   // ----> now it's 4 bytes; in old version we
use
> size_t, that's 8-bytes.

Thanks for the follow-up. That makes perfect sense. Thanks for this fix. :)