Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1633323002:
Fix possible crash in SafeStackFrameIterator (Closed)
DescriptionFix possible crash in SafeStackFrameIterator
Safe stack iterator is supposed to work even when the stack is in an inconsistent state.
E.g. during cpu profile sample recording. This patch eliminates a crash if the frame marker
is found to be bogus.
BUG=v8:4705
LOG=N
Committed: https://crrev.com/d11b44ec69c718a9b3b11872e7638c5675abd02c
Cr-Commit-Position: refs/heads/master@{#33558}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 19 (5 generated)
alph@chromium.org changed reviewers: + jochen@chromium.org, titzer@chromium.org, yurys@chromium.org
ptal
lgtm https://codereview.chromium.org/1633323002/diff/1/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1633323002/diff/1/src/frames.cc#newcode275 src/frames.cc:275: if (SingletonFor(type) == NULL) { Maybe if (type < 0 || type >= StackFrame::NUMBER_OF_TYPES) ?
https://codereview.chromium.org/1633323002/diff/1/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1633323002/diff/1/src/frames.cc#newcode275 src/frames.cc:275: if (SingletonFor(type) == NULL) { On 2016/01/26 23:55:57, yurys wrote: > Maybe if (type < 0 || type >= StackFrame::NUMBER_OF_TYPES) ? I like mine more. Moreover it is used elsewhere (line 235).
https://codereview.chromium.org/1633323002/diff/1/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1633323002/diff/1/src/frames.cc#newcode275 src/frames.cc:275: if (SingletonFor(type) == NULL) { On 2016/01/27 00:19:36, alph wrote: > On 2016/01/26 23:55:57, yurys wrote: > > Maybe if (type < 0 || type >= StackFrame::NUMBER_OF_TYPES) ? > > I like mine more. Moreover it is used elsewhere (line 235). Acknowledged.
lgtm
https://codereview.chromium.org/1633323002/diff/1/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1633323002/diff/1/src/frames.cc#newcode237 src/frames.cc:237: DCHECK(frame_); Did you run this through the trybots yet? I might have hit this one last time when adding support for WASM frames. Is it safe(r) to just return if the frame type is not known?
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1633323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1633323002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1633323002/diff/1/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/1633323002/diff/1/src/frames.cc#newcode237 src/frames.cc:237: DCHECK(frame_); On 2016/01/27 08:29:00, titzer wrote: > Did you run this through the trybots yet? I might have hit this one last time > when adding support for WASM frames. Is it safe(r) to just return if the frame > type is not known? AFAICT it cannot return NULL if the previous call was not null. Dry run passed. I'll try to land it and keep watching it.
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1633323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1633323002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix possible crash in SafeStackFrameIterator Safe stack iterator is supposed to work even when the stack is in an inconsistent state. E.g. during cpu profile sample recording. This patch eliminates a crash if the frame marker is found to be bogus. BUG=v8:4705 LOG=N ========== to ========== Fix possible crash in SafeStackFrameIterator Safe stack iterator is supposed to work even when the stack is in an inconsistent state. E.g. during cpu profile sample recording. This patch eliminates a crash if the frame marker is found to be bogus. BUG=v8:4705 LOG=N Committed: https://crrev.com/d11b44ec69c718a9b3b11872e7638c5675abd02c Cr-Commit-Position: refs/heads/master@{#33558} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/d11b44ec69c718a9b3b11872e7638c5675abd02c Cr-Commit-Position: refs/heads/master@{#33558}
Message was sent while issue was closed.
On 2016/01/27 17:41:10, commit-bot: I haz the power wrote: > Patchset 1 (id:??) landed as > https://crrev.com/d11b44ec69c718a9b3b11872e7638c5675abd02c > Cr-Commit-Position: refs/heads/master@{#33558} This helps, but after 2 seconds, it crashes here: > v8.dll!v8::internal::StandardFrame::IsArgumentsAdaptorFrame(unsigned char * fp) Line 142 C++ v8.dll!v8::internal::StackFrame::ComputeType(const v8::internal::StackFrameIteratorBase * iterator, v8::internal::StackFrame::State * state) Line 419 C++ v8.dll!v8::internal::StackFrame::GetCallerState(v8::internal::StackFrame::State * state) Line 452 C++ v8.dll!v8::internal::SafeStackFrameIterator::IsValidCaller(v8::internal::StackFrame * frame) Line 319 C++ v8.dll!v8::internal::SafeStackFrameIterator::AdvanceOneFrame() Line 271 C++ v8.dll!v8::internal::SafeStackFrameIterator::Advance() Line 339 C++ v8.dll!v8::internal::TickSample::GetStackSample(v8::internal::Isolate * isolate, const v8::RegisterState & regs, v8::internal::TickSample::RecordCEntryFrame record_c_entry_frame, void * * frames, unsigned __int64 frames_limit, v8::SampleInfo * sample_info) Line 644 C++ v8.dll!v8::internal::TickSample::Init(v8::internal::Isolate * isolate, const v8::RegisterState & regs, v8::internal::TickSample::RecordCEntryFrame record_c_entry_frame) Line 619 C++ v8.dll!v8::internal::Sampler::SampleStack(const v8::RegisterState & state) Line 718 C++ v8.dll!v8::internal::Sampler::DoSample() Line 772 C++ v8.dll!v8::internal::ProfilerEventsProcessor::Run() Line 152 C++ v8.dll!v8::base::Thread::NotifyStartedAndRun() Line 459 C++ v8.dll!v8::base::ThreadEntry(void * arg) Line 1304 C++ [External Code]
Message was sent while issue was closed.
On 2016/01/27 19:59:06, louderspace wrote: > On 2016/01/27 17:41:10, commit-bot: I haz the power wrote: > > Patchset 1 (id:??) landed as > > https://crrev.com/d11b44ec69c718a9b3b11872e7638c5675abd02c > > Cr-Commit-Position: refs/heads/master@{#33558} > > This helps, but after 2 seconds, it crashes here: > > > v8.dll!v8::internal::StandardFrame::IsArgumentsAdaptorFrame(unsigned char * > fp) Line 142 C++ > v8.dll!v8::internal::StackFrame::ComputeType(const > v8::internal::StackFrameIteratorBase * iterator, v8::internal::StackFrame::State > * state) Line 419 C++ > > v8.dll!v8::internal::StackFrame::GetCallerState(v8::internal::StackFrame::State > * state) Line 452 C++ > > v8.dll!v8::internal::SafeStackFrameIterator::IsValidCaller(v8::internal::StackFrame > * frame) Line 319 C++ > v8.dll!v8::internal::SafeStackFrameIterator::AdvanceOneFrame() Line 271 C++ > v8.dll!v8::internal::SafeStackFrameIterator::Advance() Line 339 C++ > v8.dll!v8::internal::TickSample::GetStackSample(v8::internal::Isolate * > isolate, const v8::RegisterState & regs, > v8::internal::TickSample::RecordCEntryFrame record_c_entry_frame, void * * > frames, unsigned __int64 frames_limit, v8::SampleInfo * sample_info) Line > 644 C++ > v8.dll!v8::internal::TickSample::Init(v8::internal::Isolate * isolate, const > v8::RegisterState & regs, v8::internal::TickSample::RecordCEntryFrame > record_c_entry_frame) Line 619 C++ > v8.dll!v8::internal::Sampler::SampleStack(const v8::RegisterState & state) > Line 718 C++ > v8.dll!v8::internal::Sampler::DoSample() Line 772 C++ > v8.dll!v8::internal::ProfilerEventsProcessor::Run() Line 152 C++ > v8.dll!v8::base::Thread::NotifyStartedAndRun() Line 459 C++ > v8.dll!v8::base::ThreadEntry(void * arg) Line 1304 C++ > [External Code] Yes, there are more places to fix. Could you please post the stack trace to the bug, not here. Thank you. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
