Skip to content

Commit 4a1a6fb

Browse files
committed
Fix stack walking in fixupWork() for frames with CFA padding
Running `hhvm hphp/test/quick/dv.php` on aarch64 with HHVM compiled in release mode consistently segfaults while syncing the VM state prior to producing a backtrace for a Hack exception (see gist).[1] The crash happens because fixupWork assumes that aligned native frames directly follow one another as it traverses the chain of frame pointers while looking for the first VM frame, so it ends up using an incorrect CFA for the reconstructed VM frame. In D29878492, @colavitam already warned us of the perils inherent in this assumption and suggested using the unwinder instead. Do this now. The existing unwinder logic in unwind-itanium.cpp and co. doesn't seem to be reusable here, since we're not unwinding the stack while handling a C++ exception, but we should be able to use _Unwind_Backtrace() from libgcc_s.so in a reasonably uncomplicated fashion. Remove fixupWork() from fixup.h since it's now purely internal to the implementation file. [1] https://gist.github.com/mszabo-wikia/187833ee32cd5b6f4efa8bd987a66cd4
1 parent d43b5ed commit 4a1a6fb

File tree

2 files changed

+37
-40
lines changed

2 files changed

+37
-40
lines changed

hphp/runtime/vm/jit/fixup.cpp

Lines changed: 37 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414
+----------------------------------------------------------------------+
1515
*/
1616

17+
#include <cstdint>
18+
#include <unwind.h>
19+
1720
#include "hphp/runtime/vm/jit/fixup.h"
1821

1922
#include "hphp/runtime/base/stats.h"
@@ -22,6 +25,7 @@
2225
#include "hphp/runtime/vm/jit/tc.h"
2326

2427
#include "hphp/util/configs/jit.h"
28+
#include "hphp/util/dwarf-reg.h"
2529

2630
TRACE_SET_MOD(fixup)
2731

@@ -181,49 +185,50 @@ bool processFixupForVMFrame(VMFrame frame) {
181185
return true;
182186
}
183187

184-
bool fixupWork(ActRec* nextRbp, bool soft) {
185-
assertx(Cfg::Jit::Enabled);
186-
187-
TRACE(1, "fixup(begin):\n");
188-
189-
while (true) {
190-
auto const rbp = nextRbp;
191-
nextRbp = rbp->m_sfp;
192-
193-
if (UNLIKELY(soft) && (!nextRbp || nextRbp == rbp)) return false;
194-
assertx(nextRbp && nextRbp != rbp && "Missing fixup for native call");
188+
////////////////////////////////////////////////////////////////////////////////
189+
}
195190

196-
TRACE(2, "considering frame %p, %p\n", rbp, (void*)rbp->m_savedRip);
191+
namespace detail {
192+
struct FixupWorkState {
193+
bool soft;
194+
bool synced;
195+
};
197196

198-
if (isVMFrame(nextRbp, soft)) {
199-
TRACE(2, "fixup checking vm frame %s\n",
200-
nextRbp->func()->name()->data());
201-
auto const cfa = uintptr_t(rbp) + kNativeFrameSize;
202-
auto const frame = VMFrame{nextRbp, TCA(rbp->m_savedRip), cfa};
203-
auto const res = processFixupForVMFrame(frame);
204-
if (res || LIKELY(soft)) return res;
197+
/*
198+
* Perform a fixup of the VM registers for the current stack.
199+
*/
200+
_Unwind_Reason_Code fixupWork(_Unwind_Context* context, void* arg) {
201+
auto state = static_cast<FixupWorkState*>(arg);
202+
auto const fp = reinterpret_cast<ActRec*>(_Unwind_GetGR(context, dw_reg::FP));
203+
204+
TRACE(2, "considering frame %p, %p\n", fp, (void*)fp->m_savedRip);
205+
206+
if (isVMFrame(fp, state->soft)) {
207+
const uintptr_t cfa = _Unwind_GetCFA(context);
208+
const uintptr_t rip = _Unwind_GetIP(context);
209+
auto frame = VMFrame{fp, TCA(rip), cfa};
210+
state->synced = FixupMap::processFixupForVMFrame(frame);
211+
if (state->synced || LIKELY(state->soft)) {
212+
return _URC_END_OF_STACK;
213+
}
205214
always_assert(false && "Fixup expected for leafmost VM frame");
206-
}
207215
}
208-
return false;
216+
return _URC_NO_REASON;
209217
}
210218

211-
////////////////////////////////////////////////////////////////////////////////
212-
}
213-
214-
namespace detail {
215219
void syncVMRegsWork(bool soft) {
220+
assertx(Cfg::Jit::Enabled);
221+
222+
TRACE(1, "fixup(begin):\n");
223+
216224
// Start looking for fixup entries at the current (C++) frame. This
217225
// will walk the frames upward until we find a TC frame.
218-
DECLARE_FRAME_POINTER(framePtr);
219-
auto fp = regState() >= VMRegState::GUARDED_THRESHOLD ?
220-
(ActRec*)regState() : framePtr;
226+
FixupWorkState state{soft, false};
227+
_Unwind_Backtrace(fixupWork, &state);
221228

222-
// TODO(mcolavita): This is incorrect for C++ routines with padding after
223-
// their CFA.
224-
auto const synced = FixupMap::fixupWork(fp, soft);
229+
assertx((state.synced || state.soft) && "Missing fixup for native call");
225230

226-
if (synced) regState() = VMRegState::CLEAN;
231+
if (state.synced) regState() = VMRegState::CLEAN;
227232
Stats::inc(Stats::TC_Sync);
228233
}
229234
}

hphp/runtime/vm/jit/fixup.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -218,14 +218,6 @@ size_t size();
218218
* Returns whether we successfully performed the fixup.
219219
*/
220220
bool processFixupForVMFrame(VMFrame frame);
221-
222-
/*
223-
* Perform a fixup of the VM registers for a stack whose first frame is `rbp`.
224-
*
225-
* Returns whether we successfully performed the fixup. (We assert on failure
226-
* if `soft` is not set).
227-
*/
228-
bool fixupWork(ActRec* rbp, bool soft = false);
229221
}
230222

231223
namespace detail {

0 commit comments

Comments
 (0)