Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by haraken Modified:
4 years, 10 months ago CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium) Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionBuild fix: Add NO_SANITIZE_ADDRESS to addToFreeList
This fixes crashes in ASan builds.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/builds/18946
BUG=
TBR=keishi@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197005
Patch Set 1 #
Total comments: 4
Messages
Total messages: 12 (2 generated)
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 197005 (presubmit successful).
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
lgtm, but didn't this show up in local testing? https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, size); Can't we delay this instead?
Message was sent while issue was closed.
https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, size); On 2015/06/12 07:01:23, sof wrote: > Can't we delay this instead? We shouldn't delay the poisoning :) It is important that freed-but-not-yet-added-to-freelists memory is poisoned. This enables ASan to detect (more) use-after-free errors (than the case we delay the poisoning).
Message was sent while issue was closed.
https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, size); On 2015/06/12 07:08:25, haraken wrote: > On 2015/06/12 07:01:23, sof wrote: > > Can't we delay this instead? > > We shouldn't delay the poisoning :) It is important that > freed-but-not-yet-added-to-freelists memory is poisoned. This enables ASan to > detect (more) use-after-free errors (than the case we delay the poisoning). That doesn't make sense if you immediately afterwards mutate the payload for the common code path; now all accesses are exempted from checking here (m_freeLists ?)
Message was sent while issue was closed.
https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, size); On 2015/06/12 07:11:26, sof wrote: > On 2015/06/12 07:08:25, haraken wrote: > > On 2015/06/12 07:01:23, sof wrote: > > > Can't we delay this instead? > > > > We shouldn't delay the poisoning :) It is important that > > freed-but-not-yet-added-to-freelists memory is poisoned. This enables ASan to > > detect (more) use-after-free errors (than the case we delay the poisoning). > > That doesn't make sense if you immediately afterwards mutate the payload for the > common code path; now all accesses are exempted from checking here (m_freeLists > ?) Maybe I'm not sure I get your point. - Memory in the m_freeLists must be poisoned. - Memory that is delayed reusing and not yet in the m_freeLists must be poisoned. - Memory should be unpoisoned immediately before it is reused. Isn't the current code doing that?
Message was sent while issue was closed.
On 2015/06/12 07:15:45, haraken wrote: > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... > Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, size); > On 2015/06/12 07:11:26, sof wrote: > > On 2015/06/12 07:08:25, haraken wrote: > > > On 2015/06/12 07:01:23, sof wrote: > > > > Can't we delay this instead? > > > > > > We shouldn't delay the poisoning :) It is important that > > > freed-but-not-yet-added-to-freelists memory is poisoned. This enables ASan > to > > > detect (more) use-after-free errors (than the case we delay the poisoning). > > > > That doesn't make sense if you immediately afterwards mutate the payload for > the > > common code path; now all accesses are exempted from checking here > (m_freeLists > > ?) > > Maybe I'm not sure I get your point. > > - Memory in the m_freeLists must be poisoned. > - Memory that is delayed reusing and not yet in the m_freeLists must be > poisoned. > - Memory should be unpoisoned immediately before it is reused. > > Isn't the current code doing that? It definitely should be poisoned before we leave addFreeList(), but no need to do it before you do the extra zapping of payloads here.
Message was sent while issue was closed.
On 2015/06/12 07:18:40, sof wrote: > On 2015/06/12 07:15:45, haraken wrote: > > > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp > > File Source/platform/heap/Heap.cpp (right): > > > > > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... > > Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, size); > > On 2015/06/12 07:11:26, sof wrote: > > > On 2015/06/12 07:08:25, haraken wrote: > > > > On 2015/06/12 07:01:23, sof wrote: > > > > > Can't we delay this instead? > > > > > > > > We shouldn't delay the poisoning :) It is important that > > > > freed-but-not-yet-added-to-freelists memory is poisoned. This enables ASan > > to > > > > detect (more) use-after-free errors (than the case we delay the > poisoning). > > > > > > That doesn't make sense if you immediately afterwards mutate the payload for > > the > > > common code path; now all accesses are exempted from checking here > > (m_freeLists > > > ?) > > > > Maybe I'm not sure I get your point. > > > > - Memory in the m_freeLists must be poisoned. > > - Memory that is delayed reusing and not yet in the m_freeLists must be > > poisoned. > > - Memory should be unpoisoned immediately before it is reused. > > > > Isn't the current code doing that? > > It definitely should be poisoned before we leave addFreeList(), but no need to > do it before you do the extra zapping of payloads here. Ah, I got your point :) Will fix it in a follow-up.
Message was sent while issue was closed.
On 2015/06/12 07:22:34, haraken wrote: > On 2015/06/12 07:18:40, sof wrote: > > On 2015/06/12 07:15:45, haraken wrote: > > > > > > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... > > > Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, > size); > > > On 2015/06/12 07:11:26, sof wrote: > > > > On 2015/06/12 07:08:25, haraken wrote: > > > > > On 2015/06/12 07:01:23, sof wrote: > > > > > > Can't we delay this instead? > > > > > > > > > > We shouldn't delay the poisoning :) It is important that > > > > > freed-but-not-yet-added-to-freelists memory is poisoned. This enables > ASan > > > to > > > > > detect (more) use-after-free errors (than the case we delay the > > poisoning). > > > > > > > > That doesn't make sense if you immediately afterwards mutate the payload > for > > > the > > > > common code path; now all accesses are exempted from checking here > > > (m_freeLists > > > > ?) > > > > > > Maybe I'm not sure I get your point. > > > > > > - Memory in the m_freeLists must be poisoned. > > > - Memory that is delayed reusing and not yet in the m_freeLists must be > > > poisoned. > > > - Memory should be unpoisoned immediately before it is reused. > > > > > > Isn't the current code doing that? > > > > It definitely should be poisoned before we leave addFreeList(), but no need to > > do it before you do the extra zapping of payloads here. > > Ah, I got your point :) Will fix it in a follow-up. The annotations added here aren't sufficient to quieten ASan, http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
Message was sent while issue was closed.
On 2015/06/12 08:14:07, sof wrote: > On 2015/06/12 07:22:34, haraken wrote: > > On 2015/06/12 07:18:40, sof wrote: > > > On 2015/06/12 07:15:45, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.cpp > > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1184633005/diff/1/Source/platform/heap/Heap.c... > > > > Source/platform/heap/Heap.cpp:1055: ASAN_POISON_MEMORY_REGION(address, > > size); > > > > On 2015/06/12 07:11:26, sof wrote: > > > > > On 2015/06/12 07:08:25, haraken wrote: > > > > > > On 2015/06/12 07:01:23, sof wrote: > > > > > > > Can't we delay this instead? > > > > > > > > > > > > We shouldn't delay the poisoning :) It is important that > > > > > > freed-but-not-yet-added-to-freelists memory is poisoned. This enables > > ASan > > > > to > > > > > > detect (more) use-after-free errors (than the case we delay the > > > poisoning). > > > > > > > > > > That doesn't make sense if you immediately afterwards mutate the payload > > for > > > > the > > > > > common code path; now all accesses are exempted from checking here > > > > (m_freeLists > > > > > ?) > > > > > > > > Maybe I'm not sure I get your point. > > > > > > > > - Memory in the m_freeLists must be poisoned. > > > > - Memory that is delayed reusing and not yet in the m_freeLists must be > > > > poisoned. > > > > - Memory should be unpoisoned immediately before it is reused. > > > > > > > > Isn't the current code doing that? > > > > > > It definitely should be poisoned before we leave addFreeList(), but no need > to > > > do it before you do the extra zapping of payloads here. > > > > Ah, I got your point :) Will fix it in a follow-up. > > The annotations added here aren't sufficient to quieten ASan, > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%... Let me revert them. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
