The x86 instruction unwinder can be asked to disassemble non-instruction

blocks of memory, and if the final bytes of that block look like a long
x86 instruction, it can cause the llvm disassembler to read past the end
of the buffer.  Use the maximum allowed instruction length that we pass
to the llvm disassembler as a way to limit this to the size of the buffer.

An example of how to trigger this is when lldb does a function call, it
puts a breakpoint on the beginning of main() and uses that as the return
address from the function call.  When we stop at that location, lldb may
try to find the first frame up the stack.  Because this is on the first
instruction of a function, it will get the word-size value at the stack
pointer and assume that this was the caller's pc value.  But this is random
stack memory and could point to anything - an object in memory, something
in the data section, whatever.  And if we have a symbol for that thing,
we'll try to disassemble it.

This was leading to infrequent crashes in customer scenarios; figured out
what was happening with address sanitizer.

<rdar://problem/30463256> 

llvm-svn: 307454

lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp

@@ -604,9 +604,10 @@ uint32_t x86AssemblyInspectionEngine::extract_4(uint8_t *b) {
 }
 
 bool x86AssemblyInspectionEngine::instruction_length(uint8_t *insn_p,
-                                                     int &length) {
+                                                     int &length, 
+                                                     uint32_t buffer_remaining_bytes) {
 
-  const uint32_t max_op_byte_size = m_arch.GetMaximumOpcodeByteSize();
+  uint32_t max_op_byte_size = std::min(buffer_remaining_bytes, m_arch.GetMaximumOpcodeByteSize());
   llvm::SmallVector<uint8_t, 32> opcode_data;
   opcode_data.resize(max_op_byte_size);
 
@@ -698,8 +699,9 @@ bool x86AssemblyInspectionEngine::GetNonCallSiteUnwindPlanFromAssembly(
     bool row_updated = false; // The UnwindPlan::Row 'row' has been updated
 
     m_cur_insn = data + current_func_text_offset;
-    if (!instruction_length(m_cur_insn, insn_len) || insn_len == 0 ||
-        insn_len > kMaxInstructionByteSize) {
+    if (!instruction_length(m_cur_insn, insn_len, size - current_func_text_offset)
+        || insn_len == 0 
+        || insn_len > kMaxInstructionByteSize) {
       // An unrecognized/junk instruction
       break;
     }
@@ -1002,8 +1004,9 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
   while (offset < size) {
     m_cur_insn = data + offset;
     int insn_len;
-    if (!instruction_length(m_cur_insn, insn_len) || insn_len == 0 ||
-        insn_len > kMaxInstructionByteSize) {
+    if (!instruction_length(m_cur_insn, insn_len, size - offset)
+        || insn_len == 0 
+        || insn_len > kMaxInstructionByteSize) {
       // An unrecognized/junk instruction.
       break;
     }
@@ -1214,8 +1217,9 @@ bool x86AssemblyInspectionEngine::FindFirstNonPrologueInstruction(
     int scratch;
 
     m_cur_insn = data + offset;
-    if (!instruction_length(m_cur_insn, insn_len) ||
-        insn_len > kMaxInstructionByteSize || insn_len == 0) {
+    if (!instruction_length(m_cur_insn, insn_len, size - offset) 
+        || insn_len > kMaxInstructionByteSize 
+        || insn_len == 0) {
       // An error parsing the instruction, i.e. probably data/garbage - stop
       // scanning
       break;

lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.h

@@ -113,7 +113,7 @@ private:
   bool ret_pattern_p();
   uint32_t extract_4(uint8_t *b);
 
-  bool instruction_length(uint8_t *insn, int &length);
+  bool instruction_length(uint8_t *insn, int &length, uint32_t buffer_remaining_bytes);
 
   bool machine_regno_to_lldb_regno(int machine_regno, uint32_t &lldb_regno);
 

lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

@@ -2415,3 +2415,28 @@ TEST_F(Testx86AssemblyInspectionEngine, TestStackRealign32BitDisp_x86_64) {
   EXPECT_EQ(rsp_plus_8,
             plan.GetRowForFunctionOffset(sizeof(data) - 1)->GetCFAValue());
 }
+
+// Give the disassembler random bytes to test that it doesn't exceed
+// the bounds of the array when run under clang's address sanitizer.
+TEST_F(Testx86AssemblyInspectionEngine, TestDisassemblyJunkBytes) {
+  AddressRange sample_range;
+  UnwindPlan unwind_plan(eRegisterKindLLDB);
+  std::unique_ptr<x86AssemblyInspectionEngine> engine32 = Geti386Inspector();
+  std::unique_ptr<x86AssemblyInspectionEngine> engine64 = Getx86_64Inspector();
+
+  uint8_t data[] = {
+      0x10, 0x10, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+      0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 };
+
+  sample_range = AddressRange(0x1000, sizeof(data));
+
+  EXPECT_TRUE(engine32->GetNonCallSiteUnwindPlanFromAssembly(
+      data, sizeof(data), sample_range, unwind_plan));
+
+  unwind_plan.Clear();
+
+  EXPECT_TRUE(engine64->GetNonCallSiteUnwindPlanFromAssembly(
+      data, sizeof(data), sample_range, unwind_plan));
+
+}
+