Skip to content

Commit

Permalink
Refactor visual diff action
Browse files Browse the repository at this point in the history 
- Pass `Config` to `SendGuiMessage()`
- Return `absl::Status`
- Use `absl::FunctionRef<>`
- Clean up how the path to "Program Files" is obtained

PiperOrigin-RevId: 637559132
Change-Id: I26f461a3be98305494bba3d67198ccfee5ecd9bb
  • Loading branch information
@cblichmann @copybara-github
cblichmann authored and copybara-github committed May 27, 2024
1 parent f464766 commit 25319d9
Show file tree
Hide file tree
Showing 7 changed files with 125 additions and 104 deletions.
18 changes: 17 additions & 1 deletion config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,13 @@ namespace util = ::proto2::util;
} // namespace google::protobuf
#endif

using google::protobuf::util::JsonPrintOptions;
using google::protobuf::util::JsonParseOptions;
using google::protobuf::util::JsonPrintOptions;
using google::protobuf::util::JsonStringToMessage;
using google::protobuf::util::MessageToJsonString;
using security::binexport::GetCommonAppDataDirectory;
using security::binexport::GetOrCreateAppDataDirectory;
using security::binexport::GetProgramFilesDirectory;

namespace security::bindiff::config {

Expand Down Expand Up @@ -185,4 +186,19 @@ void MergeInto(const Config& from, Config& config) {
}
}

std::string GetBinDiffDirOrDefault(const Config& from) {
if (!from.directory().empty()) {
return from.directory();
}
#if defined(_WIN32)
return JoinPath(GetProgramFilesDirectory().value_or(R"(C:\Program Files)"),
absl::StrCat("BinDiff ", kBinDiffRelease));
#elif defined(__APPLE__)
return JoinPath(*GetProgramFilesDirectory(),
"BinDiff/BinDiff.app/Contents/MacOS");
#else
return JoinPath(*GetProgramFilesDirectory(), "bindiff");
#endif
}

} // namespace security::bindiff::config
4 changes: 4 additions & 0 deletions config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#ifndef CONFIG_H_
#define CONFIG_H_

#include <string>

#include "third_party/absl/base/attributes.h"
#include "third_party/absl/status/status.h"
#include "third_party/absl/status/statusor.h"
Expand Down Expand Up @@ -51,6 +53,8 @@ absl::Status SaveUserConfig(const Config& config);
// matter.
void MergeInto(const Config& from, Config& config);

std::string GetBinDiffDirOrDefault(const Config& from);

} // namespace security::bindiff::config

#endif // CONFIG_H_
2 changes: 2 additions & 0 deletions ida/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ ida_target_link_libraries(${bindiff_ida_plugin_name}
binexport_plugin_shared
bindiff_base
bindiff_shared
absl::cleanup
absl::function_ref
absl::str_format
absl::time
)
Expand Down
89 changes: 41 additions & 48 deletions ida/main_plugin.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "third_party/zynamics/bindiff/ida/main_plugin.h"

#include <cstdio>
#include <exception>
#include <fstream>
#include <limits>
#include <memory>
Expand Down Expand Up @@ -283,26 +284,17 @@ void Plugin::VisualDiff(uint32_t index, bool call_graph_diff) {
}

LOG(INFO) << "Sending result to BinDiff GUI...";
const auto& config = config::Proto();
const auto& ui_config = config.ui();

const std::string default_bindiff_dir =
#if defined(_WIN32)
absl::StrCat("C:\\Program Files\\BinDiff ", kBinDiffRelease);
#elif defined(__APPLE__)
"/Applications/BinDiff/BinDiff.app/Contents/MacOS";
#else
"/opt/bindiff";
#endif
SendGuiMessage(
ui_config.retries() != 0 ? ui_config.retries() : 20,
!config.directory().empty() ? config.directory() : default_bindiff_dir,
!ui_config.server().empty() ? ui_config.server() : "127.0.0.1",
ui_config.port() != 0 ? ui_config.port() : 2000, message,
/*callback=*/nullptr);
} catch (const std::runtime_error& message) {
LOG(INFO) << "Error while calling BinDiff UI: " << message.what();
warning("Error while calling BinDiff UI: %s\n", message.what());
if (absl::Status status = SendGuiMessage(config::Proto(), message, [] {});
!status.ok()) {
const std::string error_message = absl::StrCat(
"Cannot launch BinDiff user interface. Process creation failed: ",
status.message());
LOG(INFO) << error_message;
warning("%s\n", error_message.c_str());
}
} catch (const std::runtime_error& error_message) {
LOG(INFO) << "Error while calling BinDiff UI: " << error_message.what();
warning("Error while calling BinDiff UI: %s\n", error_message.what());
} catch (...) {
LOG(INFO) << "Unknown error while calling BinDiff UI";
warning("Unknown error while calling BinDiff UI\n");
Expand All @@ -319,7 +311,7 @@ bool Plugin::DiscardResults(Plugin::DiscardResultsKind kind) {
ask_yn(ASKBTN_YES,
"%sCurrent diff results have not been"
" saved - save before closing?",
kind == DiscardResultsKind::kAskSave ? "HIDECANCEL\n" : "");
kind == DiscardResultsKind::kAskSave ? "HIDECANCEL\n" : "");
if (answer == ASKBTN_YES) {
DoSaveResults();
} else if (answer == ASKBTN_CANCEL) {
Expand Down Expand Up @@ -560,10 +552,10 @@ bool DoDiffDatabase(bool filtered) {
DiffAddressRange(start_address_source, end_address_source,
start_address_target, end_address_target);
if (!diffed.ok()) {
const std::string message =
const std::string error_message =
absl::StrCat("Error while diffing: ", diffed.status().message());
LOG(INFO) << message;
warning("%s\n", message.c_str());
LOG(INFO) << error_message;
warning("%s\n", error_message.c_str());
return false;
}
return *diffed;
Expand Down Expand Up @@ -625,9 +617,9 @@ bool DoPortComments() {
start_address_source, end_address_source, start_address_target,
end_address_target, min_confidence, min_similarity);
if (!status.ok()) {
const std::string message(status.message());
LOG(INFO) << "Error: " << message;
warning("Error: %s\n", message.c_str());
const std::string error_message(status.message());
LOG(INFO) << "Error: " << error_message;
warning("Error: %s\n", error_message.c_str());
return false;
}
MatchedFunctionsChooser::Refresh();
Expand Down Expand Up @@ -758,8 +750,7 @@ bool DoSaveResults() {
absl::StatusOr<std::string> filename =
GetSaveFilename("Save Results As", default_filename,
{{"BinDiff Result files", "*.BinDiff"},
{"BinDiff Groundtruth files", "*.truth"}
});
{"BinDiff Groundtruth files", "*.truth"}});
if (!filename.ok()) {
return false;
}
Expand All @@ -775,10 +766,10 @@ bool DoSaveResults() {
}

if (!status.ok()) {
std::string message =
std::string error_message =
absl::StrCat("Error writing results: ", status.message());
LOG(INFO) << message;
warning("%s\n", message.c_str());
LOG(INFO) << error_message;
warning("%s\n", error_message.c_str());
return false;
}

Expand Down Expand Up @@ -879,9 +870,9 @@ bool Plugin::LoadResults() {
LOG(INFO) << absl::StrCat("done (", HumanReadableDuration(timer.elapsed()),
")");
return true;
} catch (const std::exception& message) {
LOG(INFO) << "Error loading results: " << message.what();
warning("Error loading results: %s\n", message.what());
} catch (const std::exception& error_message) {
LOG(INFO) << "Error loading results: " << error_message.what();
warning("Error loading results: %s\n", error_message.what());
} catch (...) {
LOG(INFO) << "Error loading results.";
warning("Error loading results.");
Expand Down Expand Up @@ -1021,9 +1012,9 @@ int HandleImportSymbolsComments(action_activation_ctx_t* context,
absl::Status status = results->PortComments(
absl::MakeConstSpan(&ida_selection.front(), ida_selection.size()), kind);
if (!status.ok()) {
const std::string message(status.message());
LOG(INFO) << "Error: " << message;
warning("Error: %s\n", message.c_str());
const std::string error_message(status.message());
LOG(INFO) << "Error: " << error_message;
warning("Error: %s\n", error_message.c_str());
return 0;
}
// Need to refresh all choosers
Expand Down Expand Up @@ -1053,9 +1044,9 @@ int idaapi ConfirmMatchesAction::activate(action_activation_ctx_t* context) {
absl::Status status = results->ConfirmMatches(
absl::MakeConstSpan(&ida_selection.front(), ida_selection.size()));
if (!status.ok()) {
const std::string message(status.message());
LOG(INFO) << "Error: " << message;
warning("Error: %s\n", message.c_str());
const std::string error_message(status.message());
LOG(INFO) << "Error: " << error_message;
warning("Error: %s\n", error_message.c_str());
return 0;
}
MatchedFunctionsChooser::Refresh();
Expand All @@ -1065,9 +1056,9 @@ int idaapi ConfirmMatchesAction::activate(action_activation_ctx_t* context) {
int HandleCopyAddress(Address address) {
absl::Status status = CopyToClipboard(FormatAddress(address));
if (!status.ok()) {
const std::string message(status.message());
LOG(INFO) << "Error: " << message;
warning("Error: %s\n", message.c_str());
const std::string error_message(status.message());
LOG(INFO) << "Error: " << error_message;
warning("Error: %s\n", error_message.c_str());
return 0;
}
return 1;
Expand Down Expand Up @@ -1139,9 +1130,9 @@ class AddMatchAction : public ActionHandler<AddMatchAction> {
results->AddMatch(results->GetPrimaryAddress(index_primary),
results->GetSecondaryAddress(index_secondary));
if (!status.ok()) {
const std::string message(status.message());
LOG(INFO) << "Error: " << message;
warning("Error: %s\n", message.c_str());
const std::string error_message(status.message());
LOG(INFO) << "Error: " << error_message;
warning("Error: %s\n", error_message.c_str());
return 0;
}
MatchedFunctionsChooser::Refresh();
Expand Down Expand Up @@ -1199,13 +1190,15 @@ void Plugin::InitActions() {

MatchedFunctionsChooser::RegisterActions();

// Unmatched choosers
// Primary unmatched chooser
register_action(CopyAddressAction::MakeActionDesc(
UnmatchedFunctionsChooserPrimary::kCopyAddressAction, "Copy ~a~ddress",
/*shortcut=*/"", /*tooltip=*/"", /*icon=*/-1));
register_action(AddMatchAction::MakeActionDesc(
UnmatchedFunctionsChooserPrimary::kAddMatchAction, "Add ~m~atch",
/*shortcut=*/"", /*tooltip=*/"", /*icon=*/-1));

// Secondary unmatched chooser
register_action(CopyAddressAction::MakeActionDesc(
UnmatchedFunctionsChooserSecondary::kCopyAddressAction, "Copy ~a~ddress",
/*shortcut=*/"", /*tooltip=*/"", /*icon=*/-1));
Expand Down
25 changes: 11 additions & 14 deletions ida/results.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,7 @@ bool Results::PrepareVisualCallGraphDiff(size_t index, std::string* message) {
return false;
}

const FixedPointInfo& fixed_point_info(*indexed_fixed_points_[index]);
const FixedPointInfo& fixed_point_info = *indexed_fixed_points_[index];
++diff_database_id_;
std::string name =
absl::StrCat("visual_diff", diff_database_id_, ".database");
Expand Down Expand Up @@ -1158,19 +1158,16 @@ bool Results::PrepareVisualDiff(size_t index, std::string* message) {
return false;
}

const FixedPointInfo& fixed_point_info(*indexed_fixed_points_[index]);
const FixedPointInfo& fixed_point_info = *indexed_fixed_points_[index];

FlowGraphInfo empty;
const FlowGraphInfo& primary_info(
flow_graph_infos1_.find(fixed_point_info.primary) !=
flow_graph_infos1_.end()
? flow_graph_infos1_.find(fixed_point_info.primary)->second
: empty);
const FlowGraphInfo& secondary_info(
flow_graph_infos2_.find(fixed_point_info.secondary) !=
flow_graph_infos2_.end()
? flow_graph_infos2_.find(fixed_point_info.secondary)->second
: empty);
FlowGraphInfo empty{0};
auto primary_entry = flow_graph_infos1_.find(fixed_point_info.primary);
const FlowGraphInfo& primary_info =
primary_entry != flow_graph_infos1_.end() ? primary_entry->second : empty;
auto secondary_entry = flow_graph_infos2_.find(fixed_point_info.secondary);
const FlowGraphInfo& secondary_info =
secondary_entry != flow_graph_infos2_.end() ? secondary_entry->second
: empty;
if (primary_info.instruction_count == 0 &&
secondary_info.instruction_count == 0) {
warning("Both functions are empty, nothing to display!");
Expand Down Expand Up @@ -1233,7 +1230,7 @@ FixedPoint* Results::FindFixedPoint(const FixedPointInfo& fixed_point_info) {
return const_cast<FixedPoint*>(&fixed_point);
}
}
return 0;
return nullptr;
}

void Results::Read(Reader* reader) {
Expand Down
Loading

0 comments on commit 25319d9

Please sign in to comment.