Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414253004:
[Oilpan] Add HeapObjectHeader::gcGeneration() and related methods (Closed)
|
Created:
5 years, 1 month ago by peria Modified:
5 years, 1 month ago CC:
chromium-reviews, blink-reviews, kouhei+heap_chromium.org, oilpan-reviews, Mads Ager (chromium) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL was merged to https://codereview.chromium.org/1411603007/
HeapObjectHeader stores gcGeneration for use-after-free detection.
This CL puts gcGeneration in HeapObjectHeader on creating each
on-heap object.
A new element "gcGeneration" is put in the empty 4 Bytes space
of HeapObjectHeader, so this CL does not expand the size of
HeapObjcectHeader class.
gcGeneration added in this CL will be used in use-after-free detection.
See the bug for the details.
BUG=550325
Patch Set 1 #Patch Set 2 : Rebase from master #Patch Set 3 : Rebase #Patch Set 4 : Rename #
Total comments: 2
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Add HeapObjectHeader::gcGeneration() and related methods BUG=550325 ========== to ========== HeapObjectHeader stores gcGeneration for use-after-free detection. Stroing a timestamp in HeapObjectHeader and Oilpan handles, we can verify a handle points a live object. This CL puts gcGeneration in HeapObjectHeader on ASSERT build. BUG=550325 ==========
peria@chromium.org changed reviewers: + oilpan-reviews@chromium.org
just make it public
Description was changed from ========== HeapObjectHeader stores gcGeneration for use-after-free detection. Stroing a timestamp in HeapObjectHeader and Oilpan handles, we can verify a handle points a live object. This CL puts gcGeneration in HeapObjectHeader on ASSERT build. BUG=550325 ========== to ========== HeapObjectHeader stores gcGeneration for use-after-free detection. This CL puts gcGeneration in HeapObjectHeader on creating each on-heap object. A new element "gcGeneration" is put in the empty 4 Bytes area of HeapObjectHeader, so this CL does not expand the size of HeapObjcectHeader class. gcGeneration added in this CL will be used in use-after-free detection. See the issue for the details. BUG=550325 ==========
Description was changed from ========== HeapObjectHeader stores gcGeneration for use-after-free detection. This CL puts gcGeneration in HeapObjectHeader on creating each on-heap object. A new element "gcGeneration" is put in the empty 4 Bytes area of HeapObjectHeader, so this CL does not expand the size of HeapObjcectHeader class. gcGeneration added in this CL will be used in use-after-free detection. See the issue for the details. BUG=550325 ========== to ========== HeapObjectHeader stores gcGeneration for use-after-free detection. This CL puts gcGeneration in HeapObjectHeader on creating each on-heap object. A new element "gcGeneration" is put in the empty 4 Bytes space of HeapObjectHeader, so this CL does not expand the size of HeapObjcectHeader class. gcGeneration added in this CL will be used in use-after-free detection. See the issue for the details. BUG=550325 ==========
PTL
Description was changed from ========== HeapObjectHeader stores gcGeneration for use-after-free detection. This CL puts gcGeneration in HeapObjectHeader on creating each on-heap object. A new element "gcGeneration" is put in the empty 4 Bytes space of HeapObjectHeader, so this CL does not expand the size of HeapObjcectHeader class. gcGeneration added in this CL will be used in use-after-free detection. See the issue for the details. BUG=550325 ========== to ========== HeapObjectHeader stores gcGeneration for use-after-free detection. This CL puts gcGeneration in HeapObjectHeader on creating each on-heap object. A new element "gcGeneration" is put in the empty 4 Bytes space of HeapObjectHeader, so this CL does not expand the size of HeapObjcectHeader class. gcGeneration added in this CL will be used in use-after-free detection. See the bug for the details. BUG=550325 ==========
Without seeing how the methods are used, it's hard to review this CL (e.g., Maybe it would make more sense to move putGcGeneration() into HeapObjectHeader's constructor). So let's land this CL when you land the call sites.
On 2015/11/06 02:39:04, haraken wrote: > Without seeing how the methods are used, it's hard to review this CL (e.g., > Maybe it would make more sense to move putGcGeneration() into HeapObjectHeader's > constructor). So let's land this CL when you land the call sites. HeapObjectHeader::gcGeneration() will be used in Member, and https://codereview.chromium.org/1430983002/ will add it.
On 2015/11/09 09:05:21, peria wrote: > HeapObjectHeader::gcGeneration() will be used in Member, and > https://codereview.chromium.org/1430983002/ > will add it. This CL was updated to another CL https://codereview.chromium.org/1411603007/
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:221: void putGcGeneration(); What does this give you beyond what magic values already provide?
https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:221: void putGcGeneration(); On 2015/11/10 16:09:24, sof wrote: > What does this give you beyond what magic values already provide? They do same works to check an object is alive or not. In addition, gcGeneration is expected to check a pointer handle (Member<>) points a new different object on the same address. For example, ``` UntracedMember<A> a(new A); Heap::collectGarbage(); // GC collects 'a' UntracedMember<A> b(new A); a->foo(); ``` In this code, it is expected that `a->foo()` crashes with use-after-free. However, if 'b' is allocated on the same address with 'a', it goes successfully as if there are no issue in it. We expect that we can detect such use-after-free with gcGeneration, which was impossible with zapMagic.
On 2015/11/10 16:45:01, peria wrote: > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/HeapPage.h:221: void putGcGeneration(); > On 2015/11/10 16:09:24, sof wrote: > > What does this give you beyond what magic values already provide? > > They do same works to check an object is alive or not. > In addition, gcGeneration is expected to check a pointer handle (Member<>) > points a new different object on the same address. > > For example, > ``` > UntracedMember<A> a(new A); > Heap::collectGarbage(); // GC collects 'a' > UntracedMember<A> b(new A); > a->foo(); > ``` > In this code, it is expected that `a->foo()` crashes with use-after-free. > However, if 'b' is allocated on the same address with 'a', it goes successfully > as if there are no issue in it. > We expect that we can detect such use-after-free with gcGeneration, > which was impossible with zapMagic. Yes, zapMagic is just a marker to indicate that it is (very likely to be) a HeapObjectHeader.
On 2015/11/10 16:51:21, haraken wrote: > On 2015/11/10 16:45:01, peria wrote: > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/HeapPage.h:221: void > putGcGeneration(); > > On 2015/11/10 16:09:24, sof wrote: > > > What does this give you beyond what magic values already provide? > > > > They do same works to check an object is alive or not. > > In addition, gcGeneration is expected to check a pointer handle (Member<>) > > points a new different object on the same address. > > > > For example, > > ``` > > UntracedMember<A> a(new A); > > Heap::collectGarbage(); // GC collects 'a' > > UntracedMember<A> b(new A); > > a->foo(); > > ``` > > In this code, it is expected that `a->foo()` crashes with use-after-free. > > However, if 'b' is allocated on the same address with 'a', it goes > successfully > > as if there are no issue in it. > > We expect that we can detect such use-after-free with gcGeneration, > > which was impossible with zapMagic. > > Yes, zapMagic is just a marker to indicate that it is (very likely to be) a > HeapObjectHeader. As commented in the other CL, I'd recommend you merge this CL with the other CL. It's hard to understand this CL without looking at how it's used.
On 2015/11/10 16:45:01, peria wrote: > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/HeapPage.h:221: void putGcGeneration(); > On 2015/11/10 16:09:24, sof wrote: > > What does this give you beyond what magic values already provide? > > They do same works to check an object is alive or not. > In addition, gcGeneration is expected to check a pointer handle (Member<>) > points a new different object on the same address. > > For example, > ``` > UntracedMember<A> a(new A); > Heap::collectGarbage(); // GC collects 'a' > UntracedMember<A> b(new A); > a->foo(); > ``` > In this code, it is expected that `a->foo()` crashes with use-after-free. > However, if 'b' is allocated on the same address with 'a', it goes successfully > as if there are no issue in it. > We expect that we can detect such use-after-free with gcGeneration, > which was impossible with zapMagic. But there's no reason why the zap values have to be static? iow, why have two similar mechanisms around. If UntracedMember<> is considered rare, then the UntracedMember<> constructor could record that it is pointing into a heap page. And have the page sweeper overwrite the UntracedMember<>s that point to freed objects afterwards. Then you'd be able to detect latent pointers to freed objects, before they're even attempted accessed.
On 2015/11/10 17:58:33, sof wrote: > On 2015/11/10 16:45:01, peria wrote: > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/HeapPage.h:221: void > putGcGeneration(); > > On 2015/11/10 16:09:24, sof wrote: > > > What does this give you beyond what magic values already provide? > > > > They do same works to check an object is alive or not. > > In addition, gcGeneration is expected to check a pointer handle (Member<>) > > points a new different object on the same address. > > > > For example, > > ``` > > UntracedMember<A> a(new A); > > Heap::collectGarbage(); // GC collects 'a' > > UntracedMember<A> b(new A); > > a->foo(); > > ``` > > In this code, it is expected that `a->foo()` crashes with use-after-free. > > However, if 'b' is allocated on the same address with 'a', it goes > successfully > > as if there are no issue in it. > > We expect that we can detect such use-after-free with gcGeneration, > > which was impossible with zapMagic. > > But there's no reason why the zap values have to be static? iow, why have two > similar mechanisms around. > > If UntracedMember<> is considered rare, then the UntracedMember<> constructor > could record that it is pointing into a heap page. And have the page sweeper > overwrite the UntracedMember<>s that point to freed objects afterwards. Then > you'd be able to detect latent pointers to freed objects, before they're even > attempted accessed. Ah, makes sense.
Description was changed from ========== HeapObjectHeader stores gcGeneration for use-after-free detection. This CL puts gcGeneration in HeapObjectHeader on creating each on-heap object. A new element "gcGeneration" is put in the empty 4 Bytes space of HeapObjectHeader, so this CL does not expand the size of HeapObjcectHeader class. gcGeneration added in this CL will be used in use-after-free detection. See the bug for the details. BUG=550325 ========== to ========== This CL was merged to https://codereview.chromium.org/1411603007/ HeapObjectHeader stores gcGeneration for use-after-free detection. This CL puts gcGeneration in HeapObjectHeader on creating each on-heap object. A new element "gcGeneration" is put in the empty 4 Bytes space of HeapObjectHeader, so this CL does not expand the size of HeapObjcectHeader class. gcGeneration added in this CL will be used in use-after-free detection. See the bug for the details. BUG=550325 ==========
On 2015/11/10 17:58:33, sof wrote: > On 2015/11/10 16:45:01, peria wrote: > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/HeapPage.h:221: void > putGcGeneration(); > > On 2015/11/10 16:09:24, sof wrote: > > > What does this give you beyond what magic values already provide? > > > > They do same works to check an object is alive or not. > > In addition, gcGeneration is expected to check a pointer handle (Member<>) > > points a new different object on the same address. > > > > For example, > > ``` > > UntracedMember<A> a(new A); > > Heap::collectGarbage(); // GC collects 'a' > > UntracedMember<A> b(new A); > > a->foo(); > > ``` > > In this code, it is expected that `a->foo()` crashes with use-after-free. > > However, if 'b' is allocated on the same address with 'a', it goes > successfully > > as if there are no issue in it. > > We expect that we can detect such use-after-free with gcGeneration, > > which was impossible with zapMagic. > > But there's no reason why the zap values have to be static? iow, why have two > similar mechanisms around. Ah, right. I think we don't need two. haraken@, can we replace 'magic' with gcGeneration? Then we can use uint32 for gcGeneration. > If UntracedMember<> is considered rare, then the UntracedMember<> constructor > could record that it is pointing into a heap page. And have the page sweeper > overwrite the UntracedMember<>s that point to freed objects afterwards. Then > you'd be able to detect latent pointers to freed objects, before they're even > attempted accessed. It makes sense. And I feel it is applicable on release build! My approach with gcGeneration also checks with Member and WeakMember on ASSERT build, to catch up use-after-free of them.
On 2015/11/11 03:06:38, peria wrote: > On 2015/11/10 17:58:33, sof wrote: > > On 2015/11/10 16:45:01, peria wrote: > > > > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/heap/HeapPage.h (right): > > > > > > > > > https://codereview.chromium.org/1414253004/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/heap/HeapPage.h:221: void > > putGcGeneration(); > > > On 2015/11/10 16:09:24, sof wrote: > > > > What does this give you beyond what magic values already provide? > > > > > > They do same works to check an object is alive or not. > > > In addition, gcGeneration is expected to check a pointer handle (Member<>) > > > points a new different object on the same address. > > > > > > For example, > > > ``` > > > UntracedMember<A> a(new A); > > > Heap::collectGarbage(); // GC collects 'a' > > > UntracedMember<A> b(new A); > > > a->foo(); > > > ``` > > > In this code, it is expected that `a->foo()` crashes with use-after-free. > > > However, if 'b' is allocated on the same address with 'a', it goes > > successfully > > > as if there are no issue in it. > > > We expect that we can detect such use-after-free with gcGeneration, > > > which was impossible with zapMagic. > > > > But there's no reason why the zap values have to be static? iow, why have two > > similar mechanisms around. > > Ah, right. I think we don't need two. > haraken@, can we replace 'magic' with gcGeneration? > Then we can use uint32 for gcGeneration. Yes, I think so.
Filed a nice idea of sof@ to make light weight detector for UntracedMember<> as a feature request. http://crbug.com/555364 So let me close this CL. It is already merged into another CL http://crrev.com/1411603007 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
