Add BinDiff Similarity Provider (API) - #8387
Conversation
| { | ||
| public: | ||
| security::bindiff::CallGraph m_callGraph; | ||
| security::bindiff::FlowGraphs m_flowGraphs; |
There was a problem hiding this comment.
I believe you're leaking memory here. BinDiffView needs a destructor.
~BinDiffView()
{
security::bindiff::DeleteFlowGraphs(&m_flowGraphs);
}FlowGraphs is defined as
using FlowGraphs = std::set<FlowGraph*, SortByAddress>;Additionally copying should be disabled too or given explicit ownership semantics
BinDiffView(const BinDiffView&) = delete;
BinDiffView& operator=(const BinDiffView&) = delete;
| for (const auto& match : pendingMatches) | ||
| { | ||
| const SimilarityResultId primaryResult = | ||
| results.AddResult(match.key.primary, match.key.secondary, match.similarity, 255); |
There was a problem hiding this comment.
Couple of questions here:
- Whats the reason these are getting Max confidence this seems wrong. Could you just store confidence in pendingMatches?
- Please don't use magic values like this prefer named constants.
| const int permissions = GetSegmentPermissions(*segment); | ||
|
|
||
| // Map the segment without reading its contents. | ||
| m_impl->m_flagSpace.AddMemoryBlock(segmentAddr, AddressSpace::MemoryBlock(segmentLen), permissions); |
There was a problem hiding this comment.
We should not be doing this for non-file backed segments which can be arbitrarily large. MemoryBlock will allocate a byte for every segmentLen.
| primaryView.m_flowGraphs, secondaryView.m_flowGraphs, fixedPoints); | ||
|
|
||
| // TODO: Expose options for diffing. | ||
| security::bindiff::Diff(&context, security::bindiff::GetDefaultMatchingSteps(), |
There was a problem hiding this comment.
Non-blocking but it would be great if we could plumb our completion.IsStopRequested() into Diff as this is where I think the bulk of the time is going to be spent. Maybe its worth keeping track of this as a separate issue?
|
|
||
| bool success; | ||
| { | ||
| BinDiffProcessor processor(*view); |
There was a problem hiding this comment.
Non-blocking but I feel like this should really be done off the main thread with a progress callback. Perhaps another issue we should create.
| ## Built-in similarity | ||
|
|
||
| The **Google BinDiff** provider can also compare binaries directly in a | ||
| [similarity session](./similarity.md), without first creating intermediate BinExport files. |
There was a problem hiding this comment.
I trust similarity.md is in another PR?
|
I think the only two blocking requests are the memory exhaustion and the memory leak issues. |
2 participants
Adds a first party integration with https://github.com/google/bindiff meant for simple structural (cfg) diffing and matching, supersedes the binexport plugin.
An example of the rendering performed giving the bindiff annotations.
The sister PR is found here: https://github.com/Vector35/binaryninja/pull/1743
Depends on: