From 0eaba4ebccc997c221f6d49376ae8c11dab90831 Mon Sep 17 00:00:00 2001 From: BinDiff Authors Date: Tue, 4 Aug 2026 04:55:41 -0700 Subject: [PATCH 1/2] Automated Code Change PiperOrigin-RevId: 958953771 Change-Id: Ib9e8c2786d29ed92b08089a06c49a3893150c75f --- differ.cc | 2 +- main_portable.cc | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/differ.cc b/differ.cc index 9af911e7..f6c185c6 100644 --- a/differ.cc +++ b/differ.cc @@ -135,7 +135,7 @@ absl::Status SetupGraphsFromProto( if (proto_flow_graph.basic_block_index_size() == 0) { continue; } - auto flow_graph = absl::make_unique(); + auto flow_graph = std::make_unique(); ABSL_RETURN_IF_ERROR(flow_graph->Read(proto, proto_flow_graph, call_graph, instruction_cache)); diff --git a/main_portable.cc b/main_portable.cc index ccb570a9..b1b24458 100644 --- a/main_portable.cc +++ b/main_portable.cc @@ -284,7 +284,7 @@ void DifferThread::operator()() { if (did_handle_error(filename.status())) { continue; } - writer.Add(absl::make_unique(*filename)); + writer.Add(std::make_unique(*filename)); } if (g_output_binary) { absl::StatusOr filename = GetTruncatedFilename( @@ -648,7 +648,7 @@ absl::Status BinDiffMain(int argc, char* argv[]) { if (FileExists(primary)) { // Primary from file system. FlowGraphInfos infos; - call_graph1 = absl::make_unique(); + call_graph1 = std::make_unique(); ABSL_RETURN_IF_ERROR(Read(primary, call_graph1.get(), &flow_graphs1, &infos, &instruction_cache)); } @@ -674,7 +674,7 @@ absl::Status BinDiffMain(int argc, char* argv[]) { if (!secondary.empty() && FileExists(secondary)) { // secondary from filesystem FlowGraphInfos infos; - call_graph2 = absl::make_unique(); + call_graph2 = std::make_unique(); ABSL_RETURN_IF_ERROR(Read(secondary, call_graph2.get(), &flow_graphs2, &infos, &instruction_cache)); } From b454a27f153f4d45fac527579dc67f72ee0b9caa Mon Sep 17 00:00:00 2001 From: Christian Blichmann Date: Tue, 4 Aug 2026 06:39:26 -0700 Subject: [PATCH 2/2] Refactor `FlowGraph` and `CallGraph` creation to use factory methods - Replace the constructors of `FlowGraph` with static factory methods (`Create` and `FromProto`) that return `absl::StatusOr` - Update `CallGraph::AttachFlowGraph` and `DetachFlowGraph` to return `absl::Status` instead of throwing exceptions. Update call sites and tests to handle the new status-returning APIs. - Same exercise for the IDA Pro plugin's `SetupTemporaryFlowGraphs()` PiperOrigin-RevId: 958989365 Change-Id: Ic720e6452f20dcdd074abe26f5238dfe6ee5fe2a --- call_graph.cc | 39 ++++++------ call_graph.h | 26 +++++--- call_graph_test.cc | 27 +++------ database_writer.cc | 14 ++--- differ.cc | 20 +++---- flow_graph.cc | 136 +++++++++++++++++++---------------------- flow_graph.h | 48 +++++++++------ ida/results.cc | 146 ++++++++++++++++++++++----------------------- ida/results.h | 12 ++-- test_util.cc | 31 ++++++---- test_util.h | 42 ++++++++----- 11 files changed, 273 insertions(+), 268 deletions(-) diff --git a/call_graph.cc b/call_graph.cc index c4323d32..062f718b 100644 --- a/call_graph.cc +++ b/call_graph.cc @@ -20,11 +20,11 @@ #include #include #include -#include #include #include #include +#include "third_party/absl/log/check.h" #include "third_party/absl/log/log.h" #include "third_party/absl/status/status.h" #include "third_party/absl/strings/str_cat.h" @@ -173,45 +173,42 @@ absl::Status CallGraph::Read(const BinExport2& proto, return absl::OkStatus(); } -void CallGraph::AttachFlowGraph(FlowGraph* flow_graph) { - if (!flow_graph) { - throw std::runtime_error( - "AttachFlowGraph: invalid flow graph (null pointer)"); - } - - auto entry_point_address = flow_graph->GetEntryPointAddress(); +absl::Status CallGraph::AttachFlowGraph(FlowGraph& flow_graph) { + auto entry_point_address = flow_graph.GetEntryPointAddress(); auto vertex = GetVertex(entry_point_address); if (vertex == kInvalidVertex) { - throw std::runtime_error(absl::StrCat( - "AttachFlowGraph: couldn't find call graph node for flow graph ", - FormatAddress(entry_point_address))); + return absl::FailedPreconditionError( + absl::StrCat("AttachFlowGraph: couldn't find call graph node for flow " + "graph ", + FormatAddress(entry_point_address))); } if (graph_[vertex].flow_graph_ != nullptr) { - throw std::runtime_error( + return absl::FailedPreconditionError( absl::StrCat("AttachFlowGraph: flow graph already attached ", FormatAddress(entry_point_address))); } - graph_[vertex].flow_graph_ = flow_graph; - flow_graph->SetCallGraph(this); + graph_[vertex].flow_graph_ = &flow_graph; + flow_graph.SetCallGraph(this); + return absl::OkStatus(); } -void CallGraph::DetachFlowGraph(FlowGraph* flow_graph) { - if (!flow_graph || flow_graph->GetCallGraph() != this) { - throw std::runtime_error("DetachFlowGraph: invalid graph"); +absl::Status CallGraph::DetachFlowGraph(FlowGraph& flow_graph) { + if (flow_graph.GetCallGraph() != this) { + return absl::InternalError("DetachFlowGraph: invalid graph"); } - auto entry_point_address = flow_graph->GetEntryPointAddress(); - auto vertex = GetVertex(entry_point_address); - if (vertex == kInvalidVertex) { + auto entry_point_address = flow_graph.GetEntryPointAddress(); + if (auto vertex = GetVertex(entry_point_address); vertex == kInvalidVertex) { LOG(INFO) << absl::StrCat( "DetachFlowGraph: couldn't find call graph node for flow graph ", FormatAddress(entry_point_address)); } else { graph_[vertex].flow_graph_ = nullptr; } - flow_graph->SetCallGraph(nullptr); + flow_graph.SetCallGraph(nullptr); + return absl::OkStatus(); } CallGraph::Vertex CallGraph::GetVertex(Address address) const { diff --git a/call_graph.h b/call_graph.h index 420ea50b..0402db1c 100644 --- a/call_graph.h +++ b/call_graph.h @@ -18,6 +18,7 @@ #include // NOLINT #include #include +#include #include #include @@ -80,16 +81,20 @@ class CallGraph { // A constant denoting a non-existent vertex. static constexpr Vertex kInvalidVertex = std::numeric_limits::max(); - // Constructs an empty call graph. - CallGraph() = default; - - virtual ~CallGraph() = default; - // Reads and initializes the call graph from "proto". "filename" is passed in // and remembered for informational purposes only (we want to be able to // construct default save filenames with it for example). + static absl::StatusOr> FromProto( + const BinExport2& proto, const std::string& filename); + + // Like FromProto, but initializes an existing (possibly empty) call graph. absl::Status Read(const BinExport2& proto, const std::string& filename); + // Constructs an empty call graph. + CallGraph() = default; + + virtual ~CallGraph() = default; + // Gets just the filename part (without path or extension) passed into Read(). std::string GetFilename() const; @@ -139,8 +144,9 @@ class CallGraph { // Associates the given flow graph with the corresponding call graph vertex. // The call graph will _not_ take ownership of the flow graph! - void AttachFlowGraph(FlowGraph* flow_graph); - void DetachFlowGraph(FlowGraph* flow_graph); + absl::Status AttachFlowGraph(FlowGraph& flow_graph); + absl::Status DetachFlowGraph(FlowGraph& flow_graph); + // TODO(cblichmann): Remove!!! FlowGraph* GetFlowGraph(Address address) const; FlowGraph* GetFlowGraph(Vertex vertex) const { @@ -200,8 +206,8 @@ class CallGraph { // Accesses comments. The call graph stores these globally even for operands // because we don't want to store them multiple times for shared basic blocks. - CommentsByOperatorId& GetComments() { return comments_; } - const CommentsByOperatorId& GetComments() const { return comments_; } + CommentsByOperatorId& comments() { return comments_; } + const CommentsByOperatorId& comments() const { return comments_; } // Reduces the graph to the immediate vicinity of "edge" and recalculates MD // indices on that subgraph. The idea is to become resilient against non-local @@ -214,6 +220,8 @@ class CallGraph { void DeleteVertices(Address from, Address to); protected: + friend class CallGraphPeer; + void Init(); double CalculateProximityMdIndex(Edge edge); diff --git a/call_graph_test.cc b/call_graph_test.cc index b1bcdb91..36cc7c99 100644 --- a/call_graph_test.cc +++ b/call_graph_test.cc @@ -17,7 +17,6 @@ #include // NOLINT #include #include -#include #include #include @@ -60,21 +59,13 @@ TEST(EmptyCallGraphTest, Construction) { call_graph.SetMdIndex(47.0); EXPECT_THAT(call_graph.GetMdIndex(), Eq(47.0)); - EXPECT_THAT(call_graph.GetComments(), IsEmpty()); -} - -TEST(EmptyCallGraphTest, AddOrRemoveNullFlowGraphThrows) { - CallGraph call_graph; // Empty - - EXPECT_THROW(call_graph.AttachFlowGraph(nullptr), std::runtime_error); - EXPECT_THROW(call_graph.DetachFlowGraph(nullptr), std::runtime_error); + EXPECT_THAT(call_graph.comments(), IsEmpty()); } TEST(EmptyCallGraphDeathTest, QueryingVerticesCrashes) { CallGraph call_graph; // Empty - // These should fail in all builds - // TODO(cblichmann): Implement bound checks in debug mode. + // These should fail in all builds. EXPECT_DEATH_IF_SUPPORTED(call_graph.GetAddress(CallGraph::kInvalidVertex), ""); EXPECT_DEATH_IF_SUPPORTED(call_graph.GetMdIndex(CallGraph::kInvalidVertex), @@ -84,21 +75,19 @@ TEST(EmptyCallGraphDeathTest, QueryingVerticesCrashes) { } TEST(EmptyCallGraphTest, CrossPlatformFileBasenames) { - class CallGraphForTesting : public CallGraph { - public: - void set_filename(std::string value) { filename_ = std::move(value); } - } call_graph; // Empty + CallGraph call_graph; // Empty + CallGraphPeer call_graph_peer(call_graph); // Plain filename - call_graph.set_filename("primary.v1.test.exe"); + call_graph_peer.set_filename("primary.v1.test.exe"); EXPECT_THAT(call_graph.GetFilename(), StrEq("primary.v1.test")); // Windows style - call_graph.set_filename(R"(C:\TEMP\RE.project\primary.v1.test.exe)"); + call_graph_peer.set_filename(R"(C:\TEMP\RE.project\primary.v1.test.exe)"); EXPECT_THAT(call_graph.GetFilename(), StrEq("primary.v1.test")); // Posix style - call_graph.set_filename(R"(/tmp/RE.project/primary.v1.test.exe)"); + call_graph_peer.set_filename(R"(/tmp/RE.project/primary.v1.test.exe)"); EXPECT_THAT(call_graph.GetFilename(), StrEq("primary.v1.test")); } @@ -133,7 +122,7 @@ class SimpleCallGraphTest : public ::testing::Test { {InstructionBuilder("call func_a") .SetCallsFunction("func_a"), InstructionBuilder("ret")})})}) - .Build(&cache_)), + .Build(cache_)), call_graph_(binary_->call_graph) {} Instruction::Cache cache_; diff --git a/database_writer.cc b/database_writer.cc index 631142f0..d00044e4 100644 --- a/database_writer.cc +++ b/database_writer.cc @@ -94,17 +94,17 @@ absl::Status ReadInfos(const std::string& filename, CallGraph& call_graph, for (const auto& flow_graph_proto : proto.flow_graph()) { // Create an ephemeral FlowGraph instance to update the instruction cache // and to use it to parse the BinExport2 information. - FlowGraph flow_graph; - ABSL_RETURN_IF_ERROR(flow_graph.Read(proto, flow_graph_proto, &call_graph, - &instruction_cache)); + ABSL_ASSIGN_OR_RETURN(std::unique_ptr flow_graph, + FlowGraph::FromProto(proto, flow_graph_proto, + call_graph, instruction_cache)); Counts counts; - Count(flow_graph, &counts); - Address address = flow_graph.GetEntryPointAddress(); + Count(*flow_graph, &counts); + Address address = flow_graph->GetEntryPointAddress(); FlowGraphInfo& info = flow_graph_infos[address]; info.address = address; - info.name = &flow_graph.GetName(); - info.demangled_name = &flow_graph.GetDemangledName(); + info.name = &flow_graph->GetName(); + info.demangled_name = &flow_graph->GetDemangledName(); info.basic_block_count = counts[Counts::kBasicBlocksLibrary] + counts[Counts::kBasicBlocksNonLibrary]; info.edge_count = diff --git a/differ.cc b/differ.cc index f6c185c6..9fcabbd4 100644 --- a/differ.cc +++ b/differ.cc @@ -102,19 +102,13 @@ absl::Status AddSubsToCallGraph(CallGraph* absl_nonnull call_graph, for (auto [it, end] = boost::vertices(call_graph->GetGraph()); it != end; ++it) { const CallGraph::Vertex vertex = *it; - const Address address = call_graph->GetAddress(vertex); if (call_graph->GetFlowGraph(vertex)) { continue; } + const Address address = call_graph->GetAddress(vertex); - std::unique_ptr flow_graph; - // Temporary try-catch block. A follow-up change will refactor to - // absl::StatusOr>. - try { - flow_graph = std::make_unique(call_graph, address); - } catch (const std::runtime_error& e) { - return absl::FailedPreconditionError(e.what()); - } + ABSL_ASSIGN_OR_RETURN(std::unique_ptr flow_graph, + FlowGraph::Create(*call_graph, address)); call_graph->SetStub(vertex, true); call_graph->SetLibrary(vertex, true); if (!flow_graphs->insert(flow_graph.release()).second) { @@ -135,10 +129,10 @@ absl::Status SetupGraphsFromProto( if (proto_flow_graph.basic_block_index_size() == 0) { continue; } - auto flow_graph = std::make_unique(); - ABSL_RETURN_IF_ERROR(flow_graph->Read(proto, proto_flow_graph, call_graph, - instruction_cache)); - + ABSL_ASSIGN_OR_RETURN( + std::unique_ptr flow_graph, + FlowGraph::FromProto(proto, proto_flow_graph, *call_graph, + *instruction_cache)); Counts counts; Count(*flow_graph, &counts); diff --git a/flow_graph.cc b/flow_graph.cc index 8bd84485..f85c822d 100644 --- a/flow_graph.cc +++ b/flow_graph.cc @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,8 @@ #include "third_party/absl/log/check.h" #include "third_party/absl/log/log.h" #include "third_party/absl/status/status.h" +#include "third_party/absl/status/status_macros.h" +#include "third_party/absl/status/statusor.h" #include "third_party/absl/strings/str_cat.h" #include "third_party/zynamics/bindiff/call_graph.h" #include "third_party/zynamics/bindiff/comment.h" @@ -47,8 +50,8 @@ namespace security::bindiff { -using binexport::FormatAddress; -using binexport::GetInstructionAddress; +using ::security::binexport::FormatAddress; +using ::security::binexport::GetInstructionAddress; namespace { @@ -98,46 +101,23 @@ FlowGraph::Vertex FindVertex(const std::vector
& addresses, return FlowGraph::Vertex(std::distance(addresses.begin(), it)); } -FlowGraph::FlowGraph(CallGraph* call_graph, Address entry_point) - : graph_(), - level_for_call_(), - call_graph_(call_graph), - call_graph_vertex_(call_graph->GetVertex(entry_point)), - md_index_(0), - md_index_inverted_(0), - entry_point_address_(entry_point), - fixed_point_(0), - prime_(0), - byte_hash_(1), - string_references_(1), - instructions_(), - call_targets_(), - num_loops_(0) { - call_graph_->AttachFlowGraph(this); -} - -// instruction_cache needs to be passed in (it used to be a static member) -// because otherwise flow graphs wouldn't be thread safe. The cache has to -// be a thread local object. -FlowGraph::FlowGraph() - : graph_(), - level_for_call_(), - call_graph_(), - call_graph_vertex_(0), - md_index_(0), - md_index_inverted_(0), - entry_point_address_(0), - fixed_point_(0), - prime_(0), - byte_hash_(1), - string_references_(1), - instructions_(), - call_targets_(), - num_loops_(0) {} +// We ned to mutate call_graph, so we can't make it const here. +absl::StatusOr> FlowGraph::Create( + CallGraph& call_graph, Address entry_point) { + auto flow_graph = std::make_unique(); + flow_graph->call_graph_ = &call_graph; + flow_graph->call_graph_vertex_ = call_graph.GetVertex(entry_point); + flow_graph->entry_point_address_ = entry_point; + ABSL_RETURN_IF_ERROR(call_graph.AttachFlowGraph(*flow_graph)); + return flow_graph; +} FlowGraph::~FlowGraph() { if (call_graph_) { - call_graph_->DetachFlowGraph(this); + if (absl::Status status = call_graph_->DetachFlowGraph(*this); + !status.ok()) { + LOG(ERROR) << status; + } } } @@ -226,26 +206,30 @@ int GetInternalCommentOperandNum(int operand_num, Comment::Type type, return operand_num; } -absl::Status FlowGraph::Read(const BinExport2& proto, - const BinExport2::FlowGraph& proto_flow_graph, - CallGraph* call_graph, - Instruction::Cache* instruction_cache) { - entry_point_address_ = - proto - .instruction( - proto.basic_block(proto_flow_graph.entry_basic_block_index()) - .instruction_index(0) - .begin_index()) - .address(); - call_graph_ = call_graph; - call_graph_->AttachFlowGraph(this); - call_graph_vertex_ = call_graph_->GetVertex(entry_point_address_); - - prime_ = 0; // Sum of basic block primes. - - // TODO(cblichmann): We don't export string references yet (BinDetego doesn't +// instruction_cache needs to be passed in because otherwise flow graphs +// wouldn't be thread safe. The cache has to be a thread local object. +absl::StatusOr> FlowGraph::FromProto( + const BinExport2& proto, const BinExport2::FlowGraph& proto_flow_graph, + CallGraph& call_graph, Instruction::Cache& instruction_cache) { + ABSL_ASSIGN_OR_RETURN( + auto flow_graph, + FlowGraph::Create( + call_graph, + proto + .instruction( + proto.basic_block(proto_flow_graph.entry_basic_block_index()) + .instruction_index(0) + .begin_index()) + .address())); + + flow_graph->call_graph_vertex_ = + call_graph.GetVertex(flow_graph->entry_point_address_); + + flow_graph->prime_ = 0; // Sum of basic block primes. + + // TODO(cblichmann): We don't export string references (BinDetego doesn't // have them, only the IDA plugin). - string_references_ = 1; + flow_graph->string_references_ = 1; Address computed_instruction_address = 0; int last_instruction_index = 0; @@ -253,7 +237,7 @@ absl::Status FlowGraph::Read(const BinExport2& proto, std::vector temp_vertices( proto_flow_graph.basic_block_index_size()); std::vector
temp_addresses(temp_vertices.size()); - auto& comments = call_graph_->GetComments(); + auto& comments = call_graph.comments(); for (int basic_block_index = 0; basic_block_index < proto_flow_graph.basic_block_index_size(); ++basic_block_index) { @@ -262,7 +246,7 @@ absl::Status FlowGraph::Read(const BinExport2& proto, std::string basic_block_bytes; VertexInfo& vertex_info(temp_vertices[basic_block_index]); - vertex_info.instruction_start_ = instructions_.size(); + vertex_info.instruction_start_ = flow_graph->instructions_.size(); vertex_info.prime_ = 0; // Sum of instruction primes. vertex_info.fixed_point_ = 0; // Not a fixed point yet... @@ -301,17 +285,18 @@ absl::Status FlowGraph::Read(const BinExport2& proto, proto.mnemonic(proto_instruction.mnemonic_index()).name()); const uint32_t instruction_prime = bindiff::GetPrime(mnemonic); vertex_info.prime_ += instruction_prime; - instructions_.emplace_back(instruction_cache, instruction_address, - mnemonic, instruction_prime); + flow_graph->instructions_.emplace_back(&instruction_cache, + instruction_address, mnemonic, + instruction_prime); basic_block_bytes += proto_instruction.raw_bytes(); if (proto_instruction.call_target_size() > 0 && vertex_info.call_target_start_ == std::numeric_limits::max()) { - vertex_info.call_target_start_ = call_targets_.size(); + vertex_info.call_target_start_ = flow_graph->call_targets_.size(); } for (int i = 0; i < proto_instruction.call_target_size(); ++i) { - call_targets_.push_back(proto_instruction.call_target(i)); + flow_graph->call_targets_.push_back(proto_instruction.call_target(i)); } for (const auto& comment_index : proto_instruction.comment_index()) { @@ -333,13 +318,13 @@ absl::Status FlowGraph::Read(const BinExport2& proto, } temp_addresses[basic_block_index] = - instructions_[vertex_info.instruction_start_].GetAddress(); - prime_ += vertex_info.prime_; + flow_graph->instructions_[vertex_info.instruction_start_].GetAddress(); + flow_graph->prime_ += vertex_info.prime_; vertex_info.basic_block_hash_ = GetSdbmHash(basic_block_bytes); function_bytes += basic_block_bytes; } - byte_hash_ = GetSdbmHash(function_bytes); + flow_graph->byte_hash_ = GetSdbmHash(function_bytes); if (!std::is_sorted(temp_addresses.begin(), temp_addresses.end())) { return absl::FailedPreconditionError("Basic blocks not sorted by address"); @@ -367,27 +352,28 @@ absl::Status FlowGraph::Read(const BinExport2& proto, // This leaves prime, byte hash etc unaffected. It's debatable whether that is // good or bad. It doesn't reflect the current reality of the loaded graph // after truncation, but it does reflect the actual disassembly. - if (instructions_.size() >= kMaxFunctionInstructions || + if (flow_graph->instructions_.size() >= kMaxFunctionInstructions || edges.size() >= kMaxFunctionEdges || temp_addresses.size() >= kMaxFunctionBasicBlocks) { LOG(WARNING) << absl::StrCat( - "Function ", FormatAddress(entry_point_address_), + "Function ", FormatAddress(flow_graph->entry_point_address_), " is excessively large: ", temp_addresses.size(), " basic blocks, ", - edges.size(), " edges, ", instructions_.size(), + edges.size(), " edges, ", flow_graph->instructions_.size(), " instructions. Discarding."); } else { Graph temp_graph(boost::edges_are_unsorted_multi_pass, edges.begin(), edges.end(), edge_properties.begin(), temp_addresses.size()); - std::swap(graph_, temp_graph); + std::swap(flow_graph->graph_, temp_graph); int j = 0; - for (auto [it, end] = boost::vertices(graph_); it != end; ++it, ++j) - graph_[*it] = temp_vertices[j]; + for (auto [it, end] = boost::vertices(flow_graph->graph_); it != end; + ++it, ++j) + flow_graph->graph_[*it] = temp_vertices[j]; } - Init(); - return absl::OkStatus(); + flow_graph->Init(); + return flow_graph; } void FlowGraph::Init() { diff --git a/flow_graph.h b/flow_graph.h index ef32857e..a19aa2ee 100644 --- a/flow_graph.h +++ b/flow_graph.h @@ -19,12 +19,14 @@ #include #include #include +#include #include #include #include #include #include "third_party/absl/status/status.h" +#include "third_party/absl/status/statusor.h" #include "third_party/zynamics/bindiff/call_graph.h" #include "third_party/zynamics/bindiff/graph_util.h" #include "third_party/zynamics/bindiff/instruction.h" @@ -36,8 +38,6 @@ namespace security::bindiff { class FixedPoint; class BasicBlockFixedPoint; -bool IsSorted(const std::vector
& addresses); - class FlowGraph { public: struct VertexInfo { @@ -91,17 +91,25 @@ class FlowGraph { // The lower bits are used to indicate matching steps. }; - FlowGraph(); - FlowGraph(CallGraph* call_graph, Address entry_point); - virtual ~FlowGraph(); + static absl::StatusOr> Create( + CallGraph& call_graph, Address entry_point); - // Read and initialize flow graph from given proto message. The instruction + // Reads and initializes flow graph from given proto message. The instruction // cache should be shared between flow graphs and stores mnemonic strings and // operand trees. - absl::Status Read(const BinExport2& proto, - const BinExport2::FlowGraph& proto_flow_graph, - CallGraph* call_graph, - Instruction::Cache* instruction_cache); + static absl::StatusOr> FromProto( + const BinExport2& proto, const BinExport2::FlowGraph& proto_flow_graph, + CallGraph& call_graph, Instruction::Cache& instruction_cache); + + FlowGraph() = default; + + FlowGraph(const FlowGraph&) = delete; + FlowGraph& operator=(const FlowGraph&) = delete; + + FlowGraph(FlowGraph&&) = delete; + FlowGraph& operator=(FlowGraph&&) = delete; + + virtual ~FlowGraph(); // O(logn) binary search for the vertex (==basic block) starting at "address". Vertex GetVertex(Address address) const; @@ -232,23 +240,25 @@ class FlowGraph { protected: using AddressToLevelMap = std::vector>; + friend class FlowGraphPeer; + void Init(); void MarkLoops(); Graph graph_; AddressToLevelMap level_for_call_; - CallGraph* call_graph_; + CallGraph* call_graph_ = nullptr; CallGraph::Vertex call_graph_vertex_; - double md_index_; - double md_index_inverted_; - Address entry_point_address_; - FixedPoint* fixed_point_; - uint64_t prime_; - uint32_t byte_hash_; - uint32_t string_references_; + double md_index_ = 0.0; + double md_index_inverted_ = 0.0; + Address entry_point_address_ = 0; + FixedPoint* fixed_point_ = nullptr; + uint64_t prime_ = 0; + uint32_t byte_hash_ = 1; + uint32_t string_references_ = 1; Instructions instructions_; CallTargets call_targets_; - uint16_t num_loops_; + uint16_t num_loops_ = 0; }; struct SortByAddress { diff --git a/ida/results.cc b/ida/results.cc index 33210358..ba798e6e 100644 --- a/ida/results.cc +++ b/ida/results.cc @@ -89,21 +89,19 @@ using binexport::ToStringView; namespace { -absl::Status ReadTemporaryFlowGraph(Address address, - const FlowGraphInfos& flow_graph_infos, - CallGraph* call_graph, - FlowGraph* flow_graph, - Instruction::Cache* instruction_cache) { +absl::StatusOr> ReadTemporaryFlowGraph( + Address address, const FlowGraphInfos& flow_graph_infos, + CallGraph& call_graph, Instruction::Cache& instruction_cache) { auto info = flow_graph_infos.find(address); if (info == flow_graph_infos.end()) { return absl::NotFoundError(absl::StrCat("Flow graph not found for address", FormatAddress(address))); } - std::ifstream stream(call_graph->GetFilePath(), std::ios::binary); + std::ifstream stream(call_graph.GetFilePath(), std::ios::binary); BinExport2 proto; if (!proto.ParseFromIstream(&stream)) { return absl::UnknownError(absl::StrCat( - "Failed parsing protocol buffer for ", call_graph->GetFilePath())); + "Failed parsing protocol buffer for ", call_graph.GetFilePath())); } for (const auto& proto_flow_graph : proto.flow_graph()) { // Entry point address is always set. @@ -115,9 +113,11 @@ absl::Status ReadTemporaryFlowGraph(Address address, .begin_index()) .address(); if (address == info->second.address) { - flow_graph->SetCallGraph(call_graph); - return flow_graph->Read(proto, proto_flow_graph, call_graph, - instruction_cache); + ABSL_ASSIGN_OR_RETURN( + auto flow_graph, FlowGraph::FromProto(proto, proto_flow_graph, + call_graph, instruction_cache)); + flow_graph->SetCallGraph(&call_graph); + return flow_graph; } } return absl::UnknownError( @@ -824,25 +824,28 @@ absl::Status Results::AddMatch(Address primary, Address secondary) { // Results have been loaded: we need to reload flow graphs and recreate // basic block fixed points. if (is_incomplete()) { - FlowGraph primary_graph; - FlowGraph secondary_graph; FixedPoint fixed_point; - SetupTemporaryFlowGraphs(fixed_point_info, primary_graph, secondary_graph, - fixed_point, true); + std::unique_ptr primary_graph; + std::unique_ptr secondary_graph; + ABSL_RETURN_IF_ERROR(SetupTemporaryFlowGraphs( + fixed_point_info, fixed_point, primary_graph, secondary_graph, + /*create_instruction_matches=*/true)); Counts counts; Histogram histogram; - FlowGraphs dummy1; - dummy1.insert(&primary_graph); - FlowGraphs dummy2; - dummy2.insert(&secondary_graph); - FixedPoints dummy3; - dummy3.insert(fixed_point); - GetCountsAndHistogram(dummy1, dummy2, dummy3, &histogram, &counts); + FlowGraphs hist_flow_graphs_primary; + hist_flow_graphs_primary.insert(primary_graph.get()); + FlowGraphs hist_flow_graphs_secondary; + hist_flow_graphs_secondary.insert(secondary_graph.get()); + FixedPoints hist_fixed_points; + hist_fixed_points.insert(fixed_point); + GetCountsAndHistogram(hist_flow_graphs_primary, + hist_flow_graphs_secondary, hist_fixed_points, + &histogram, &counts); fixed_point.SetMatchingStep(MatchingStep::kFunctionManualName); fixed_point.SetSimilarity(GetSimilarityScore( - primary_graph, secondary_graph, histogram, counts)); + *primary_graph, *secondary_graph, histogram, counts)); ClassifyChanges(&fixed_point); fixed_point_info.basic_block_count = counts[Counts::kBasicBlockMatchesLibrary] + @@ -1088,49 +1091,44 @@ void Results::ReadBasicblockMatches(FixedPoint* fixed_point) { } } -void Results::SetupTemporaryFlowGraphs(const FixedPointInfo& fixed_point_info, - FlowGraph& primary, FlowGraph& secondary, - FixedPoint& fixed_point, - bool create_instruction_matches) { - // TODO(cblichmann): Cache the temporary flow graphs. Comment porting should - // not need to re-parse the full BinExport2 for each match. - // In the BinExport1 format, it was necessary and efficient - // to it this way. - instruction_cache_.clear(); - if (auto status = - ReadTemporaryFlowGraph(fixed_point_info.primary, flow_graph_infos1_, - &call_graph1_, &primary, &instruction_cache_); - !status.ok()) { - throw std::runtime_error(std::string(status.message())); - } - if (auto status = ReadTemporaryFlowGraph(fixed_point_info.secondary, - flow_graph_infos2_, &call_graph2_, - &secondary, &instruction_cache_); - !status.ok()) { - throw std::runtime_error(std::string(status.message())); - } - fixed_point.Create(&primary, &secondary); +absl::Status Results::SetupTemporaryFlowGraphs( + const FixedPointInfo& fixed_point_info, FixedPoint& fixed_point, + std::unique_ptr& primary, std::unique_ptr& secondary, + bool create_instruction_matches) { + ABSL_ASSIGN_OR_RETURN( + primary, + ReadTemporaryFlowGraph(fixed_point_info.primary, flow_graph_infos1_, + call_graph1_, instruction_cache_)); + ABSL_ASSIGN_OR_RETURN( + secondary, + ReadTemporaryFlowGraph(fixed_point_info.secondary, flow_graph_infos2_, + call_graph2_, instruction_cache_)); + + fixed_point.Create(primary.get(), secondary.get()); MatchingContext context(call_graph1_, call_graph2_, flow_graphs1_, flow_graphs2_, fixed_points_); flow_graphs1_.clear(); - flow_graphs1_.insert(&primary); + flow_graphs1_.insert(primary.get()); flow_graphs2_.clear(); - flow_graphs2_.insert(&secondary); + flow_graphs2_.insert(secondary.get()); fixed_points_.clear(); fixed_point.SetConfidence(fixed_point_info.confidence); fixed_point.SetSimilarity(fixed_point_info.similarity); fixed_point.SetFlags(fixed_point_info.flags); fixed_point.SetMatchingStep(*fixed_point_info.algorithm); - std::pair fixed_point_it = - fixed_points_.insert(fixed_point); - primary.SetFixedPoint(const_cast(&*fixed_point_it.first)); - secondary.SetFixedPoint(const_cast(&*fixed_point_it.first)); + + auto [it, inserted] = fixed_points_.insert(fixed_point); + primary->SetFixedPoint(const_cast(&*it)); + secondary->SetFixedPoint(const_cast(&*it)); + if (create_instruction_matches) { FindFixedPointsBasicBlock(&fixed_point, &context, GetDefaultMatchingStepsBasicBlock()); } else { ReadBasicblockMatches(&fixed_point); } + + return absl::OkStatus(); } void Results::DeleteTemporaryFlowGraphs() { @@ -1202,14 +1200,18 @@ bool Results::PrepareVisualDiff(size_t index, std::string* message) { FlowGraphs flow_graphs1; FlowGraphs flow_graphs2; FixedPoints fixed_points; - FlowGraph primary; - FlowGraph secondary; if (is_incomplete()) { LOG(INFO) << "Loading incomplete flow graphs"; // Results have been loaded: we need to reload flow graphs and recreate // basic block fixed_points. - SetupTemporaryFlowGraphs(fixed_point_info, primary, secondary, fixed_point, - /*create_instruction_matches=*/false); + std::unique_ptr primary; + std::unique_ptr secondary; + if (auto status = SetupTemporaryFlowGraphs( + fixed_point_info, fixed_point, primary, secondary, + /*create_instruction_matches=*/false); + !status.ok()) { + throw std::runtime_error(std::string(status.message())); + } } else { fixed_point = *FindFixedPoint(fixed_point_info); } @@ -1363,14 +1365,13 @@ absl::Status Results::PortComments(Address start_address_source, for (auto* fixed_point_info : indexed_fixed_points_) { if (get_func(static_cast(fixed_point_info->primary))) { if (is_incomplete()) { - FlowGraph primary; - FlowGraph secondary; FixedPoint fixed_point; - SetupTemporaryFlowGraphs(*fixed_point_info, primary, secondary, - fixed_point, - /*create_instruction_matches=*/false); - - SetComments(&fixed_point, call_graph2_.GetComments(), + std::unique_ptr primary; + std::unique_ptr secondary; + ABSL_RETURN_IF_ERROR(SetupTemporaryFlowGraphs( + *fixed_point_info, fixed_point, primary, secondary, + /*create_instruction_matches=*/false)); + SetComments(&fixed_point, call_graph2_.comments(), start_address_target, end_address_target, start_address_source, end_address_source, min_confidence, min_similarity); @@ -1378,7 +1379,7 @@ absl::Status Results::PortComments(Address start_address_source, DeleteTemporaryFlowGraphs(); } else { SetComments(FindFixedPoint(*fixed_point_info), - call_graph2_.GetComments(), start_address_target, + call_graph2_.comments(), start_address_target, end_address_target, start_address_source, end_address_source, min_confidence, min_similarity); } @@ -1417,26 +1418,25 @@ absl::Status Results::PortComments(absl::Span indices, function->flags |= FUNC_LIB; } if (is_incomplete()) { - FlowGraph primary; - FlowGraph secondary; FixedPoint fixed_point; + std::unique_ptr primary; + std::unique_ptr secondary; // TODO(cblichmann): See comment in SetupTemporaryFlowGraphs(), cache // the BinExport2. - SetupTemporaryFlowGraphs(fixed_point_info, primary, secondary, - fixed_point, - /*create_instruction_matches=*/false); - - SetComments(&fixed_point, call_graph2_.GetComments(), + ABSL_RETURN_IF_ERROR(SetupTemporaryFlowGraphs( + fixed_point_info, fixed_point, primary, secondary, + /*create_instruction_matches=*/false)); + SetComments(&fixed_point, call_graph2_.comments(), start_address_target, end_address_target, start_address_source, end_address_source, /*min_confidence=*/0.0, /*min_similarity=*/0.0); DeleteTemporaryFlowGraphs(); } else { - SetComments(FindFixedPoint(fixed_point_info), - call_graph2_.GetComments(), start_address_target, - end_address_target, start_address_source, - end_address_source, /*min_confidence=*/0.0, + SetComments(FindFixedPoint(fixed_point_info), call_graph2_.comments(), + start_address_target, end_address_target, + start_address_source, end_address_source, + /*min_confidence=*/0.0, /*min_similarity=*/0.0); } } diff --git a/ida/results.h b/ida/results.h index a2a17c51..d89fd5f8 100644 --- a/ida/results.h +++ b/ida/results.h @@ -169,11 +169,15 @@ class Results { const IndexedFlowGraphs& flow_graphs, size_t index) const; void InitializeIndexedVectors(); void Count(); - void SetupTemporaryFlowGraphs(const FixedPointInfo& fixed_point_info, - FlowGraph& primary, FlowGraph& secondary, - FixedPoint& fixed_point, - bool create_instruction_matches); + + absl::Status SetupTemporaryFlowGraphs(const FixedPointInfo& fixed_point_info, + FixedPoint& fixed_point, + std::unique_ptr& primary, + std::unique_ptr& secondary, + bool create_instruction_matches); + void DeleteTemporaryFlowGraphs(); + FixedPoint* FindFixedPoint(const FixedPointInfo& info); void ReadBasicblockMatches(FixedPoint* fixed_point); void MarkPortedCommentsInTempDatabase(); diff --git a/test_util.cc b/test_util.cc index b4b9a638..0e2318a8 100644 --- a/test_util.cc +++ b/test_util.cc @@ -121,8 +121,8 @@ void FunctionBuilder::InitInstructions() { } } -std::unique_ptr FunctionBuilder::Build(TestCallGraph* call_graph, - Instruction::Cache* cache) { +std::unique_ptr FunctionBuilder::Build(CallGraph& call_graph, + Instruction::Cache& cache) { using Graph = FlowGraph::Graph; using VertexInfo = FlowGraph::VertexInfo; using EdgeInfo = FlowGraph::EdgeInfo; @@ -131,7 +131,13 @@ std::unique_ptr FunctionBuilder::Build(TestCallGraph* call_graph, std::vector vertices; InitInstructions(); - auto flow_graph = absl::make_unique(call_graph, entry_point_); + std::unique_ptr flow_graph; + if (auto graph = FlowGraph::Create(call_graph, entry_point_); graph.ok()) { + flow_graph = std::move(*graph); + } else { + return nullptr; + } + FlowGraphPeer flow_graph_peer(*flow_graph); std::vector> edges; std::vector properties; @@ -145,9 +151,9 @@ std::unique_ptr FunctionBuilder::Build(TestCallGraph* call_graph, labels[basic_block.label_] = label_id++; for (auto& instruction : basic_block.instructions_) { ++instruction_offset; - flow_graph->instructions_.emplace_back(cache, instruction.address_, - instruction.mnemonic_, - instruction.prime_); + flow_graph_peer.instructions().emplace_back(&cache, instruction.address_, + instruction.mnemonic_, + instruction.prime_); vertex->prime_ += instruction.prime_; } } @@ -176,12 +182,12 @@ std::unique_ptr FunctionBuilder::Build(TestCallGraph* call_graph, graph[*it] = vertices[j]; } - flow_graph->Init(); + flow_graph_peer.Init(); return flow_graph; } std::unique_ptr DiffBinaryBuilder::Build( - Instruction::Cache* cache) { + Instruction::Cache& cache) { using Graph = CallGraph::Graph; using EdgeInfo = CallGraph::EdgeInfo; @@ -225,11 +231,12 @@ std::unique_ptr DiffBinaryBuilder::Build( vertex.address_ = func_it->entry_point_; vertex.name_ = std::move(func_it->name_); } - diff_binary->call_graph.Init(); + CallGraphPeer call_graph_peer(diff_binary->call_graph); + call_graph_peer.Init(); for (auto& function : functions_) { diff_binary->flow_graphs.insert( - function.Build(&diff_binary->call_graph, cache).release()); + function.Build(diff_binary->call_graph, cache).release()); } return diff_binary; } @@ -257,7 +264,7 @@ void BinDiffTest::SetUpBasicFunctions() { .SetFlow("loc_10005"), BasicBlockBuilder("loc_10005") .AddInstructions({InstructionBuilder("ret")})})}) - .Build(&cache_); + .Build(cache_); secondary_ = DiffBinaryBuilder() .AddFunctions( @@ -280,7 +287,7 @@ void BinDiffTest::SetUpBasicFunctions() { .SetFlow("loc_20005"), BasicBlockBuilder("loc_20005") .AddInstructions({InstructionBuilder("ret")})})}) - .Build(&cache_); + .Build(cache_); } void BinDiffTest::SetUpBasicFunctionMatch() { diff --git a/test_util.h b/test_util.h index af2d376d..17c01513 100644 --- a/test_util.h +++ b/test_util.h @@ -42,19 +42,29 @@ class BinDiffEnvironment : public ::testing::Environment { void SetUp() override; }; -// Call graph class that exposes more parts of its internal API for testing. -class TestCallGraph : public CallGraph { - public: - using CallGraph::CallGraph; - using CallGraph::Init; +// Helper class to access private members of CallGraph for testing. +struct CallGraphPeer { + explicit CallGraphPeer(CallGraph& call_graph) : call_graph(call_graph) {} + + void Init() { call_graph.Init(); } + + void set_filename(absl::string_view filename) { + call_graph.filename_ = filename; + } + + CallGraph& call_graph; }; -// Similar to TestCallGraph, a flow graph class exposing protected members. -class TestFlowGraph : public FlowGraph { - public: - using FlowGraph::FlowGraph; - using FlowGraph::Init; - using FlowGraph::instructions_; +// Helper class to access private members of FlowGraph for testing, similar to +// CallGraphPeer. +struct FlowGraphPeer { + explicit FlowGraphPeer(FlowGraph& flow_graph) : flow_graph(flow_graph) {} + + void Init() { flow_graph.Init(); } + + Instructions& instructions() { return flow_graph.instructions_; } + + FlowGraph& flow_graph; }; class InstructionBuilder { @@ -126,8 +136,8 @@ class FunctionBuilder { return *this; } - std::unique_ptr Build(TestCallGraph* call_graph, - Instruction::Cache* cache); + std::unique_ptr Build(CallGraph& call_graph, + Instruction::Cache& cache); private: friend class DiffBinaryBuilder; @@ -145,7 +155,7 @@ struct DiffBinary { ~DiffBinary(); Instruction::Cache* cache; - TestCallGraph call_graph; + CallGraph call_graph; FlowGraphs flow_graphs; }; @@ -157,7 +167,7 @@ class DiffBinaryBuilder { return *this; } - std::unique_ptr Build(Instruction::Cache* cache); + std::unique_ptr Build(Instruction::Cache& cache); private: std::vector functions_; @@ -165,7 +175,7 @@ class DiffBinaryBuilder { class BinDiffTest : public ::testing::Test { protected: - // Sets up this test with BinDiff strutures corresponding to two simple + // Sets up this test with BinDiff structures corresponding to two simple // functions that are matched using "manual" matching. // This can be used in tests that just need simple BinDiff context ensure // basic functionality works.