Refactor logging and use plain msg() in IDA

This prevents a dead-lock with the log executor that surfaced when building with IDA SDK 7.6. Additionally, the log executor was not really necessary at all, as `msg()`/`vmsg()` are marked as being thread-safe. Also remove `executable` and `executable64` from BinDiff config. Those were never actually used by the Java UI in released versions and we do not support using BinDiff with `idat`/`idat64` anyways. This change also actually implements file logging in the plugins if requested. Before, this could only be requested on the command-line. Other chages: - Suppress MSVC compiler warnings in IDA SDK, disable some globally PiperOrigin-RevId: 374132575 Change-Id: I74c31664a5f2aad1c1bda907521b6028c8a8c8ab
google · May 17, 2021 · 44d5b5f · 44d5b5f
1 parent bd31841
commit 44d5b5f
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 54 deletions.
diff --git a/cmake/CompileOptions.cmake b/cmake/CompileOptions.cmake
@@ -27,8 +27,11 @@ if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")  # GCC
 elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang")  # Clang or Apple Clang
   add_compile_options(-Wno-nullability-completeness)
 elseif(MSVC)  # Visual Studio
-  add_definitions(-D_CRT_SECURE_NO_WARNINGS)
-
+  add_compile_options(
+    /wd4018  # signed/unsigned mismatch
+    /wd4244  # 'argument' conversion, possible loss of data
+    /wd4267  # 'initializing' conversion, possible loss of data
+  )
   # Use the static runtime
   foreach(flag_var CMAKE_C_FLAGS CMAKE_CXX_FLAGS
                    CMAKE_C_FLAGS_DEBUG CMAKE_CXX_FLAGS_DEBUG
@@ -60,6 +63,7 @@ if(UNIX)
   endif()
 elseif(WIN32)
   add_definitions(
+    -D_CRT_SECURE_NO_WARNINGS
     # Protobuf iterators trigger deprecation warnings
     -D_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS
     # Do not define min/max macros which collide with std::min()/std::max()

diff --git a/ida/begin_idasdk.inc b/ida/begin_idasdk.inc
@@ -44,6 +44,10 @@
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wvarargs"
 #elif _MSC_VER
+#pragma warning(push)
+#pragma warning(disable : 4005)
+#pragma warning(disable : 4244)
+#pragma warning(disable : 4267)
 #endif
 
 // Now include the problematic header, end_idasdk.inc will clean up again.

diff --git a/ida/end_idasdk.inc b/ida/end_idasdk.inc
@@ -20,6 +20,7 @@
 #elif __GNUC__
 #pragma GCC diagnostic pop
 #elif _MSC_VER
+#pragma warning(pop)
 #endif
 
 #undef uint128

diff --git a/ida/log_sink.cc b/ida/log_sink.cc
@@ -14,6 +14,8 @@
 
 #include "third_party/zynamics/binexport/ida/log_sink.h"
 
+#include <algorithm>
+
 // clang-format off
 #include "third_party/zynamics/binexport/ida/begin_idasdk.inc"  // NOLINT
 #include <kernwin.hpp>                                          // NOLINT
@@ -27,20 +29,12 @@
 namespace security::binexport {
 
 void IdaLogSink::Send(const not_absl::LogEntry& entry) {
-  struct LoggingExecutor : public exec_request_t {
-    explicit LoggingExecutor(absl::string_view text_message)
-        : text_message(text_message) {}
-
-    int idaapi execute() override {
-      msg("%s\n", text_message.c_str());
-      return 0;
-    }
-
-    std::string text_message;
-  } executor(entry.text_message());
-
-  // Do all logging in IDA's main thread.
-  execute_sync(executor, MFF_FAST);
+  auto message = entry.text_message();
+  // Output up to 8K of log data (including the new line).
+  // Note: msg() and vmsg() are thread-safe.
+  msg("%.*s\n",
+      static_cast<int>(std::clamp(message.size(), size_t(0), size_t(8191))),
+      message.data());
 }
 
 }  // namespace security::binexport
diff --git a/ida/main_plugin.cc b/ida/main_plugin.cc
@@ -333,9 +333,10 @@ ssize_t idaapi UiHook(void*, int event_id, va_list arguments) {
 Plugin::LoadStatus Plugin::Init() {
   alsologtostderr_ =
       absl::AsciiStrToUpper(GetArgument("AlsoLogToStdErr")) == "TRUE";
+  log_filename_ = GetArgument("LogFile");
   if (auto status = InitLogging(LoggingOptions{}
                                     .set_alsologtostderr(alsologtostderr_)
-                                    .set_log_filename(GetArgument("LogFile")),
+                                    .set_log_filename(log_filename_),
                                 absl::make_unique<IdaLogSink>());
       !status.ok()) {
     LOG(INFO) << "Error initializing logging, skipping BinExport plugin";

diff --git a/ida/main_plugin.h b/ida/main_plugin.h
@@ -15,6 +15,8 @@
 #ifndef IDA_MAIN_PLUGIN_H_
 #define IDA_MAIN_PLUGIN_H_
 
+#include <string>
+
 #include "third_party/zynamics/binexport/ida/plugin.h"
 
 namespace security::binexport {
@@ -30,11 +32,13 @@ class Plugin : public IdaPlugin<Plugin> {
   void Terminate() override;
 
   bool alsologtostderr() const { return alsologtostderr_; }
+  const std::string& log_filename() const { return log_filename_; }
 
   bool x86_noreturn_heuristic() const { return x86_noreturn_heuristic_; }
 
  private:
   bool alsologtostderr_ = false;
+  std::string log_filename_;
 
   // Whether to use an X86-specific heuristic to identify functions that do not
   // return. See FlowGraph::FindBasicBlockBreaks() for details.

diff --git a/util/filesystem_test.cc b/util/filesystem_test.cc
@@ -61,7 +61,7 @@ TEST(FileSystemTest, Filenames) {
   EXPECT_THAT(ReplaceFileExtension(
                   absl::StrCat("subdir", kPathSeparator, "filename.ext"), ""),
               StrEq(absl::StrCat("subdir", kPathSeparator, "filename")));
-  // Test that directories with a "." in them don't throw of extension
+  // Test that directories with a "." in them don't throw off the extension
   // replacement.
   EXPECT_THAT(
       ReplaceFileExtension(

diff --git a/util/idb_export.cc b/util/idb_export.cc
@@ -59,38 +59,36 @@ absl::Status ExportDatabase(const std::string& idb_path,
     return absl::NotFoundError(absl::StrCat("File not found: " + idb_path));
   }
 
-  std::string ida_exe = is_64bit ? options.ida_exe64 : options.ida_exe;
-  if (ida_exe.empty()) {
 #ifdef _WIN32
-    ida_exe = is_64bit ? "ida64.exe" : "ida.exe";
+  const std::string ida_exe = is_64bit ? "ida64.exe" : "ida.exe";
 #else
-    ida_exe = is_64bit ? "ida64" : "ida";
+  const std::string ida_exe = is_64bit ? "ida64" : "ida";
 #endif
+  std::vector<std::string> args;
+  args.push_back(JoinPath(options.ida_dir, ida_exe));
+  args.push_back("-A");
+  args.push_back("-OBinExportAutoAction:BinExportBinary");
+  args.push_back(absl::StrCat(
+      "-OBinExportModule:",
+      // Make the output name deterministic. When only specifying a directory,
+      // BinExport will use the IDB's original executable name as the base name.
+      JoinPath(options.export_dir,
+               ReplaceFileExtension(Basename(idb_path), kBinExportExtension))));
+  args.push_back(absl::StrCat("-OBinExportAlsoLogToStdErr:",
+                              OptionBool(options.alsologtostderr)));
+  if (!options.log_filename.empty()) {
+    args.push_back(absl::StrCat("-OBinExportLogFile:", options.log_filename));
   }
-  std::vector<std::string> args = {
-      JoinPath(options.ida_dir, ida_exe),
-      "-A",
-      absl::StrCat("-OBinExportModule:",
-                   // Make the output name deterministic. When only using
-                   // specifying a directory, BinExport will use the IDB's
-                   // original executable name as the base name.
-                   JoinPath(options.export_dir,
-                            ReplaceFileExtension(Basename(idb_path),
-                                                 kBinExportExtension))),
-      absl::StrCat("-OBinExportX86NoReturnHeuristic:",
-                   OptionBool(options.x86_noreturn_heuristic)),
-      absl::StrCat("-OBinExportAlsoLogToStdErr:",
-                   OptionBool(options.alsologtostderr)),
-      "-OBinExportAutoAction:BinExportBinary",
-      idb_path,
-  };
+  args.push_back(absl::StrCat("-OBinExportX86NoReturnHeuristic:",
+                              OptionBool(options.x86_noreturn_heuristic)));
+  args.push_back(idb_path);
 
   SetEnvironmentVariable("TVHEADLESS", "1");
-  absl::StatusOr<int> exit_or = SpawnProcessAndWait(args);
+  absl::StatusOr<int> exit = SpawnProcessAndWait(args);
 
   // Reset environment variable.
   SetEnvironmentVariable("TVHEADLESS", /*value=*/"");
-  return exit_or.status();
+  return exit.status();
 }
 
 absl::Status IdbExporter::Export(
@@ -105,7 +103,7 @@ absl::Status IdbExporter::Export(
       while (true) {
         std::string idb_path;
         {
-          absl::MutexLock lock{&queue_mutex_};
+          absl::MutexLock lock(&queue_mutex_);
           if (idb_paths_.empty()) {
             break;
           }

diff --git a/util/idb_export.h b/util/idb_export.h
@@ -46,16 +46,6 @@ class IdbExporter {
       return *this;
     }
 
-    Options& set_ida_exe(std::string value) {
-      ida_exe = std::move(value);
-      return *this;
-    }
-
-    Options& set_ida_exe64(std::string value) {
-      ida_exe64 = std::move(value);
-      return *this;
-    }
-
     Options& set_num_threads(int value) {
       num_threads = value;
       return *this;
@@ -66,17 +56,21 @@ class IdbExporter {
       return *this;
     }
 
+    Options& set_log_filename(std::string value) {
+      log_filename = std::move(value);
+      return *this;
+    }
+
     Options& set_x86_noreturn_heuristic(bool value) {
       x86_noreturn_heuristic = value;
       return *this;
     }
 
     std::string export_dir;  // Directory to export the files to
     std::string ida_dir;     // IDA Pro installation directory
-    std::string ida_exe;     // Name of the IDA executable, 32-bit addresses
-    std::string ida_exe64;   // IDA executable, 64-bit addresses
     int num_threads = 1;
     bool alsologtostderr = false;
+    std::string log_filename;
     bool x86_noreturn_heuristic = false;
   };