Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(203)

Side by Side Diff: src/processor/stackwalker_amd64.cc

Issue 1902783002: Make x86-64 frame pointer unwinding stricter (Closed) Base URL: https://chromium.googlesource.com/breakpad/breakpad.git@master
Patch Set: Created 4 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
« no previous file with comments | « no previous file | src/processor/stackwalker_amd64_unittest.cc » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 // Copyright (c) 2010 Google Inc. 1 // Copyright (c) 2010 Google Inc.
2 // All rights reserved. 2 // All rights reserved.
3 // 3 //
4 // Redistribution and use in source and binary forms, with or without 4 // Redistribution and use in source and binary forms, with or without
5 // modification, are permitted provided that the following conditions are 5 // modification, are permitted provided that the following conditions are
6 // met: 6 // met:
7 // 7 //
8 // * Redistributions of source code must retain the above copyright 8 // * Redistributions of source code must retain the above copyright
9 // notice, this list of conditions and the following disclaimer. 9 // notice, this list of conditions and the following disclaimer.
10 // * Redistributions in binary form must reproduce the above 10 // * Redistributions in binary form must reproduce the above
(...skipping 146 matching lines...) Expand 10 before | Expand all | Expand 10 after
157 // If the new stack pointer is at a lower address than the old, then 157 // If the new stack pointer is at a lower address than the old, then
158 // that's clearly incorrect. Treat this as end-of-stack to enforce 158 // that's clearly incorrect. Treat this as end-of-stack to enforce
159 // progress and avoid infinite loops. 159 // progress and avoid infinite loops.
160 if (caller_rsp < callee_rsp) { 160 if (caller_rsp < callee_rsp) {
161 return true; 161 return true;
162 } 162 }
163 163
164 return false; 164 return false;
165 } 165 }
166 166
167 // Returns true if `ptr` is not in x86-64 canonical form.
168 // https://en.wikipedia.org/wiki/X86-64#Virtual_address_space_details
169 static bool is_non_canonical(uint64_t ptr) {
170 return ptr > 0x7FFFFFFFFFFF && ptr < 0xFFFF800000000000;
171 }
172
167 StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery( 173 StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery(
168 const vector<StackFrame*>& frames) { 174 const vector<StackFrame*>& frames) {
169 StackFrameAMD64* last_frame = static_cast<StackFrameAMD64*>(frames.back()); 175 StackFrameAMD64* last_frame = static_cast<StackFrameAMD64*>(frames.back());
170 uint64_t last_rsp = last_frame->context.rsp; 176 uint64_t last_rsp = last_frame->context.rsp;
171 uint64_t last_rbp = last_frame->context.rbp; 177 uint64_t last_rbp = last_frame->context.rbp;
172 178
173 // Assume the presence of a frame pointer. This is not mandated by the 179 // Assume the presence of a frame pointer. This is not mandated by the
174 // AMD64 ABI, c.f. section 3.2.2 footnote 7, though it is typical for 180 // AMD64 ABI, c.f. section 3.2.2 footnote 7, though it is typical for
175 // compilers to still preserve the frame pointer and not treat %rbp as a 181 // compilers to still preserve the frame pointer and not treat %rbp as a
176 // general purpose register. 182 // general purpose register.
177 // 183 //
178 // With this assumption, the CALL instruction pushes the return address 184 // With this assumption, the CALL instruction pushes the return address
179 // onto the stack and sets %rip to the procedure to enter. The procedure 185 // onto the stack and sets %rip to the procedure to enter. The procedure
180 // then establishes the stack frame with a prologue that PUSHes the current 186 // then establishes the stack frame with a prologue that PUSHes the current
181 // %rbp onto the stack, MOVes the current %rsp to %rbp, and then allocates 187 // %rbp onto the stack, MOVes the current %rsp to %rbp, and then allocates
182 // space for any local variables. Using this procedure linking information, 188 // space for any local variables. Using this procedure linking information,
183 // it is possible to locate frame information for the callee: 189 // it is possible to locate frame information for the callee:
184 // 190 //
185 // %caller_rsp = *(%callee_rbp + 16) 191 // %caller_rsp = *(%callee_rbp + 16)
186 // %caller_rip = *(%callee_rbp + 8) 192 // %caller_rip = *(%callee_rbp + 8)
187 // %caller_rbp = *(%callee_rbp) 193 // %caller_rbp = *(%callee_rbp)
188 194
195 // If rbp is not 8-byte aligned it can't be a frame pointer.
196 if (last_rbp % 8 != 0) {
197 return NULL;
198 }
199
189 uint64_t caller_rip, caller_rbp; 200 uint64_t caller_rip, caller_rbp;
190 if (memory_->GetMemoryAtAddress(last_rbp + 8, &caller_rip) && 201 if (memory_->GetMemoryAtAddress(last_rbp + 8, &caller_rip) &&
191 memory_->GetMemoryAtAddress(last_rbp, &caller_rbp)) { 202 memory_->GetMemoryAtAddress(last_rbp, &caller_rbp)) {
192 uint64_t caller_rsp = last_rbp + 16; 203 uint64_t caller_rsp = last_rbp + 16;
193 204
205 // If the recovered rip is not a canonical address it can't be
206 // the return address, so rbp must not have been a frame pointer.
207 if (is_non_canonical(caller_rip)) {
208 return NULL;
209 }
210
194 // Simple sanity check that the stack is growing downwards as expected. 211 // Simple sanity check that the stack is growing downwards as expected.
195 if (IsEndOfStack(caller_rip, caller_rsp, last_rsp) || 212 if (IsEndOfStack(caller_rip, caller_rsp, last_rsp) ||
196 caller_rbp < last_rbp) { 213 caller_rbp < last_rbp) {
197 // Reached end-of-stack or stack is not growing downwards. 214 // Reached end-of-stack or stack is not growing downwards.
198 return NULL; 215 return NULL;
199 } 216 }
200 217
201 StackFrameAMD64* frame = new StackFrameAMD64(); 218 StackFrameAMD64* frame = new StackFrameAMD64();
202 frame->trust = StackFrame::FRAME_TRUST_FP; 219 frame->trust = StackFrame::FRAME_TRUST_FP;
203 frame->context = last_frame->context; 220 frame->context = last_frame->context;
(...skipping 110 matching lines...) Expand 10 before | Expand all | Expand 10 after
314 // after the CALL that caused us to arrive at the callee. Set 331 // after the CALL that caused us to arrive at the callee. Set
315 // new_frame->instruction to one less than that, so it points within the 332 // new_frame->instruction to one less than that, so it points within the
316 // CALL instruction. See StackFrame::instruction for details, and 333 // CALL instruction. See StackFrame::instruction for details, and
317 // StackFrameAMD64::ReturnAddress. 334 // StackFrameAMD64::ReturnAddress.
318 new_frame->instruction = new_frame->context.rip - 1; 335 new_frame->instruction = new_frame->context.rip - 1;
319 336
320 return new_frame.release(); 337 return new_frame.release();
321 } 338 }
322 339
323 } // namespace google_breakpad 340 } // namespace google_breakpad
OLDNEW
« no previous file with comments | « no previous file | src/processor/stackwalker_amd64_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698