google / binexport

Export disassemblies into Protocol Buffers
Apache License 2.0
1.02k stars 194 forks source link

Binary Ninja: diverging instructions not being treated as such #100

Closed comex closed 1 year ago

comex commented 1 year ago

Instructions such as ret are being treated as if they flow into the next instruction.

Instruction::Instruction automatically sets FLAG_FLOW if next_instruction is valid. In the IDA exporter, next_instruction is chosen by looking for xrefs from the current instruction, so diverging instructions would have next_instruction == 0, which is invalid, causing FLAG_FLOW to not be set. But in the Binary Ninja exporter, we just have:

  // TODO(cblichmann): Is this always the case in Binja?
  const Address next_instruction = address + instruction.length;

So it always gets set.

The code does already check for regular control flow later on, so it might be easiest to just reset FLAG_FLOW at that point:

--- a/binaryninja/main_plugin.cc
+++ b/binaryninja/main_plugin.cc
@@ -245,7 +245,9 @@ void AnalyzeFlow(
   if (has_flow) {
     // Regular code flow
     entry_point_manager->Add(instruction->GetNextInstruction(),
                              EntryPoint::Source::CODE_FLOW);
+  } else {
+    instruction->SetFlag(FLAG_FLOW, false);
   }

   const std::vector<BinaryNinja::ReferenceSource> xrefs =
cblichmann commented 1 year ago

This should be fixed now. Thanks for the report.