Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2203053003:
Check stack pointer to be inside the stack when unwinding on Linux. (Closed)
|
Created:
4 years, 4 months ago by Dmitry Skiba Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck stack pointer to be inside the stack when unwinding on Linux.
On Linux unwinding stack using frame pointers relies on heuristics, which
sometimes fail to catch invalid stack pointers. However it's hard to get
stack boundaries for all threads on Linux, see the bug.
This CL checks stack pointers against real stack boundary for the main thread
only, which is enough to fix recently discovered crash (crbug.com/617730#c1).
BUG=617730
Committed: https://crrev.com/2ff54586e0102659c1034d58269ac376d1aacc20
Cr-Commit-Position: refs/heads/master@{#415065}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase; address comments #
Total comments: 2
Patch Set 3 : Address comment #Messages
Total messages: 23 (12 generated)
dskiba@google.com changed reviewers: + primiano@chromium.org, thakis@chromium.org
Code lg, I just think you don't need the extra HAVE_GET_STACK_END and you can make this more readable, see comments below. https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (left): https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#o... base/debug/stack_trace.cc:14: #if HAVE_TRACE_STACK_FRAME_POINTERS && defined(OS_ANDROID) I think you still need OS_POSIX otherwise that include pthread.h will fail on Win https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:55: static uintptr_t GetStackEnd() { I think all this become more readable and easy to thinkg about if instead of the extra HAVE_GET_STACK_END and #ifdef you do: - Always define GetStackEnd(). - Put the #ifdef inside, so it will look like: static uintptr_t GetStackEnd() { bool is_main_thread = GetCurrentProcId() == PlatformThread::CurrentId(); #if defined(OS_ANDROID) bla bla pthread #elif defined(OS_LINUX) && defined(__GLIBC__) bla bla __libc bla #else return std::numeric_limits bla } https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:91: #if defined(__GLIBC__) Just change lines 89-91 to be: #elif defined(OS_LINUX) && defined(__GLIBC__) https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:95: extern "C" void* __libc_stack_end; Ok it seems we already depend on this in StackFrameDepth.cpp, so you are not really adding a new libc symbol dependency, good.
https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (left): https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#o... base/debug/stack_trace.cc:14: #if HAVE_TRACE_STACK_FRAME_POINTERS && defined(OS_ANDROID) On 2016/08/03 09:32:03, Primiano Tucci wrote: > I think you still need OS_POSIX otherwise that include pthread.h will fail on > Win Done. https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:55: static uintptr_t GetStackEnd() { On 2016/08/03 09:32:03, Primiano Tucci wrote: > I think all this become more readable and easy to thinkg about if instead of the > extra HAVE_GET_STACK_END and #ifdef you do: > > - Always define GetStackEnd(). > - Put the #ifdef inside, so it will look like: > > static uintptr_t GetStackEnd() { > bool is_main_thread = GetCurrentProcId() == PlatformThread::CurrentId(); > > #if defined(OS_ANDROID) > bla bla pthread > #elif defined(OS_LINUX) && defined(__GLIBC__) > bla bla __libc bla > #else > return std::numeric_limits bla > } > > Done. https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:91: #if defined(__GLIBC__) On 2016/08/03 09:32:03, Primiano Tucci wrote: > Just change lines 89-91 to be: > > #elif defined(OS_LINUX) && defined(__GLIBC__) Done. https://codereview.chromium.org/2203053003/diff/1/base/debug/stack_trace.cc#n... base/debug/stack_trace.cc:95: extern "C" void* __libc_stack_end; On 2016/08/03 09:32:04, Primiano Tucci wrote: > Ok it seems we already depend on this in StackFrameDepth.cpp, so you are not > really adding a new libc symbol dependency, good. Acknowledged.
The CQ bit was checked by dskiba@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, sorry for the delay, slowly catching up with my codereview queue. LGTM. https://codereview.chromium.org/2203053003/diff/20001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2203053003/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:103: #else if you make this an #endif, you can have a unique return ...max; statement, and avoid duplicating that in lines 97-101 above (just let it fall through)
https://codereview.chromium.org/2203053003/diff/20001/base/debug/stack_trace.cc File base/debug/stack_trace.cc (right): https://codereview.chromium.org/2203053003/diff/20001/base/debug/stack_trace.... base/debug/stack_trace.cc:103: #else On 2016/08/25 11:03:18, Primiano Tucci wrote: > if you make this an #endif, you can have a unique return ...max; statement, and > avoid duplicating that in lines 97-101 above (just let it fall through) Done.
lgtm
The CQ bit was checked by dskiba@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2203053003/#ps40001 (title: "Address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dskiba@google.com
The CQ bit was unchecked by dskiba@google.com
The CQ bit was checked by dskiba@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Check stack pointer to be inside the stack when unwinding on Linux. On Linux unwinding stack using frame pointers relies on heuristics, which sometimes fail to catch invalid stack pointers. However it's hard to get stack boundaries for all threads on Linux, see the bug. This CL checks stack pointers against real stack boundary for the main thread only, which is enough to fix recently discovered crash (crbug.com/617730#c1). BUG=617730 ========== to ========== Check stack pointer to be inside the stack when unwinding on Linux. On Linux unwinding stack using frame pointers relies on heuristics, which sometimes fail to catch invalid stack pointers. However it's hard to get stack boundaries for all threads on Linux, see the bug. This CL checks stack pointers against real stack boundary for the main thread only, which is enough to fix recently discovered crash (crbug.com/617730#c1). BUG=617730 Committed: https://crrev.com/2ff54586e0102659c1034d58269ac376d1aacc20 Cr-Commit-Position: refs/heads/master@{#415065} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2ff54586e0102659c1034d58269ac376d1aacc20 Cr-Commit-Position: refs/heads/master@{#415065} |
