From 42a6039125ece238a0287c58c192e23197cc599e Mon Sep 17 00:00:00 2001 From: Jordan DeLong Date: Thu, 16 May 2013 11:42:27 -0700 Subject: [PATCH] Fix a bug with debug_backtrace during an unwind The change to stop zeroing locals during RetC was too aggressive---we stopped doing it during unwinding also. When unwinding, we need to zero locals and $this because another destructing object may still run debug_backtrace, and we won't have the *pc == OpRet{C,V} trick to tell us to ignore the junk. --- hphp/runtime/vm/bytecode.cpp | 7 +++- hphp/runtime/vm/event_hook.cpp | 2 +- hphp/runtime/vm/runtime.h | 35 +++++++++++++++++--- hphp/test/quick/unwind_backtrace.php | 32 ++++++++++++++++++ hphp/test/quick/unwind_backtrace.php.expectf | 5 +++ 5 files changed, 75 insertions(+), 6 deletions(-) create mode 100644 hphp/test/quick/unwind_backtrace.php create mode 100644 hphp/test/quick/unwind_backtrace.php.expectf diff --git a/hphp/runtime/vm/bytecode.cpp b/hphp/runtime/vm/bytecode.cpp index 4610cbae4..ed7a47462 100644 --- a/hphp/runtime/vm/bytecode.cpp +++ b/hphp/runtime/vm/bytecode.cpp @@ -1026,10 +1026,15 @@ UnwindStatus Stack::unwindFrag(ActRec* fp, int offset, * * - Finally, the exit hook for the returning function can * throw, but this happens last so everything is destructed. + * */ if (!unwindingReturningFrame) { try { - frame_free_locals_inl(fp, func->numLocals()); + // Note that we must convert locals and the $this to + // uninit/zero during unwind. This is because a backtrace + // from another destructing object during this unwind may try + // to read them. + frame_free_locals_unwind(fp, func->numLocals()); } catch (...) {} } ndiscard(func->numSlotsInFrame()); diff --git a/hphp/runtime/vm/event_hook.cpp b/hphp/runtime/vm/event_hook.cpp index 34ee2da81..2dfbfcc09 100644 --- a/hphp/runtime/vm/event_hook.cpp +++ b/hphp/runtime/vm/event_hook.cpp @@ -164,7 +164,7 @@ bool EventHook::RunInterceptHandler(ActRec* ar) { Variant ret = vm_call_user_func(h->asCArrRef()[0], intArgs); if (doneFlag.toBoolean()) { - frame_free_locals_inl_no_hook(ar, ar->m_func->numLocals()); + frame_free_locals_inl_no_hook(ar, ar->m_func->numLocals()); Stack& stack = g_vmContext->getStack(); stack.top() = (Cell*)(ar + 1); tvDup(ret.asTypedValue(), stack.allocTV()); diff --git a/hphp/runtime/vm/runtime.h b/hphp/runtime/vm/runtime.h index 8122b7b5c..5e567fd7f 100644 --- a/hphp/runtime/vm/runtime.h +++ b/hphp/runtime/vm/runtime.h @@ -81,6 +81,16 @@ frame_local_inner(const ActRec* fp, int n) { return ret->m_type == KindOfRef ? ret->m_data.pref->tv() : ret; } +/* + * 'Unwinding' versions of the below frame_free_locals_* functions + * zero locals and the $this pointer. + * + * This is necessary during unwinding because another object being + * destructed by the unwind may decide to do a debug_backtrace and + * read a destructed value. + */ + +template inline void ALWAYS_INLINE frame_free_locals_helper_inl(ActRec* fp, int numLocals) { assert(numLocals == fp->m_func->numLocals()); @@ -106,32 +116,46 @@ frame_free_locals_helper_inl(ActRec* fp, int numLocals) { DataType t = loc->m_type; if (IS_REFCOUNTED_TYPE(t)) { uint64_t datum = loc->m_data.num; + if (unwinding) { + tvWriteUninit(loc); + } tvDecRefHelper(t, datum); } } } +template inline void ALWAYS_INLINE frame_free_locals_inl_no_hook(ActRec* fp, int numLocals) { if (fp->hasThis()) { ObjectData* this_ = fp->getThis(); + if (unwinding) { + fp->setThis(nullptr); + } decRefObj(this_); } - frame_free_locals_helper_inl(fp, numLocals); + frame_free_locals_helper_inl(fp, numLocals); } inline void ALWAYS_INLINE frame_free_locals_inl(ActRec* fp, int numLocals) { - frame_free_locals_inl_no_hook(fp, numLocals); + frame_free_locals_inl_no_hook(fp, numLocals); + EventHook::FunctionExit(fp); +} + +inline void ALWAYS_INLINE +frame_free_locals_unwind(ActRec* fp, int numLocals) { + frame_free_locals_inl_no_hook(fp, numLocals); EventHook::FunctionExit(fp); } inline void ALWAYS_INLINE frame_free_locals_no_this_inl(ActRec* fp, int numLocals) { - frame_free_locals_helper_inl(fp, numLocals); + frame_free_locals_helper_inl(fp, numLocals); EventHook::FunctionExit(fp); } +// Helper for iopFCallBuiltin. inline void ALWAYS_INLINE frame_free_args(TypedValue* args, int count) { for (int i = 0; i < count; i++) { @@ -139,10 +163,13 @@ frame_free_args(TypedValue* args, int count) { DataType t = loc->m_type; if (IS_REFCOUNTED_TYPE(t)) { uint64_t datum = loc->m_data.num; + // We don't have to write KindOfUninit here, because a + // debug_backtrace wouldn't be able to see these slots (they are + // stack cells). But note we're also relying on the destructors + // not throwing. tvDecRefHelper(t, datum); } } - } Unit* diff --git a/hphp/test/quick/unwind_backtrace.php b/hphp/test/quick/unwind_backtrace.php new file mode 100644 index 000000000..6df374244 --- /dev/null +++ b/hphp/test/quick/unwind_backtrace.php @@ -0,0 +1,32 @@ +foo(); +} + +main(); + diff --git a/hphp/test/quick/unwind_backtrace.php.expectf b/hphp/test/quick/unwind_backtrace.php.expectf new file mode 100644 index 000000000..ac26f495b --- /dev/null +++ b/hphp/test/quick/unwind_backtrace.php.expectf @@ -0,0 +1,5 @@ +wat +~something +HipHop Notice: Undefined index: object in %s on line 10 +NULL +HipHop Fatal error: Uncaught exception 'Exception' with message 'asd' in %s