Look for http redirection errors from SymSrv in google_converter.

Change-Id: Ic793f2a5baceb342154c995c43bf60b6f57612a5
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3689705
Reviewed-by: Ivan Penkov <ivanpe@chromium.org>

src/tools/windows/converter/ms_symbol_server_converter.cc

@@ -128,11 +128,15 @@ bool GUIDOrSignatureIdentifier::InitializeFromString(
 #undef SSCANF
 
 MSSymbolServerConverter::MSSymbolServerConverter(
-    const string& local_cache, const vector<string>& symbol_servers)
+    const string& local_cache,
+    const vector<string>& symbol_servers,
+    bool trace_symsrv)
     : symbol_path_(),
       fail_dns_(false),
       fail_timeout_(false),
-      fail_not_found_(false) {
+      fail_http_https_redir_(false),
+      fail_not_found_(false),
+      trace_symsrv_(trace_symsrv) {
   // Setting local_cache can be done without verifying that it exists because
   // SymSrv will create it if it is missing - any creation failures will occur
   // at that time, so there's nothing to check here, making it safe to
@@ -342,6 +346,10 @@ MSSymbolServerConverter::LocateFile(const string& debug_or_code_file,
         return LOCATE_RETRY;
       }
 
+      if (fail_http_https_redir_) {
+        return LOCATE_HTTP_HTTPS_REDIR;
+      }
+
       // This is an authoritiative file-not-found message.
       if (fail_not_found_) {
         fprintf(stderr,
@@ -425,6 +433,10 @@ BOOL CALLBACK MSSymbolServerConverter::SymCallback(HANDLE process,
       // message does not use the entire string but is appended to the URL
       // that SymSrv attempted to retrieve.
       string desc(cba_event->desc);
+      if (self->trace_symsrv_) {
+        fprintf(stderr, "LocateFile: SymCallback: action desc '%s'\n",
+                desc.c_str());
+      }
 
       // desc_action maps strings (in desc) to boolean pointers that are to
       // be set to true if the string matches.
@@ -436,17 +448,22 @@ BOOL CALLBACK MSSymbolServerConverter::SymCallback(HANDLE process,
       static const desc_action desc_actions[] = {
           // When a DNS error occurs, it could be indiciative of network
           // problems.
-        { "SYMSRV:  The server name or address could not be resolved\n",
+          {"SYMSRV:  The server name or address could not be resolved\n",
-          &self->fail_dns_ },
+           &self->fail_dns_},
 
           // This message is produced if no connection is opened.
-        { "SYMSRV:  A connection with the server could not be established\n",
+          {"SYMSRV:  A connection with the server could not be established\n",
-          &self->fail_timeout_ },
+           &self->fail_timeout_},
 
           // This message is produced if a connection is established but the
           // server fails to respond to the HTTP request.
-        { "SYMSRV:  The operation timed out\n",
+          {"SYMSRV:  The operation timed out\n", &self->fail_timeout_},
-          &self->fail_timeout_ },
+
+          // This message is produced if the server is redirecting us from http
+          // to https. When this happens SymSrv will fail and we need to use
+          // the https URL in our call-- we've made a mistake.
+          {"ERROR_INTERNET_HTTP_TO_HTTPS_ON_REDIR\n",
+           &self->fail_http_https_redir_},
 
           // This message is produced when the requested file is not found,
           // even if one or more of the above messages are also produced.
@@ -454,9 +471,7 @@ BOOL CALLBACK MSSymbolServerConverter::SymCallback(HANDLE process,
           // conditions.  Note that this message will not be produced if a
           // connection is established and the server begins to respond to the
           // HTTP request but does not finish transmitting the file.
-        { " not found\n",
+          {" not found\n", &self->fail_not_found_}};
-          &self->fail_not_found_ }
-      };
 
       for (int desc_action_index = 0;
            desc_action_index <

src/tools/windows/converter/ms_symbol_server_converter.h

@@ -129,7 +129,8 @@ class MSSymbolServerConverter {
     LOCATE_FAILURE = 0,
     LOCATE_NOT_FOUND,  // Authoritative: the file is not present.
     LOCATE_RETRY,      // Transient (network?) error, try again later.
-    LOCATE_SUCCESS
+    LOCATE_SUCCESS,
+    LOCATE_HTTP_HTTPS_REDIR
   };
 
   // Create a new object.  local_cache is the location (pathname) of a local
@@ -141,8 +142,11 @@ class MSSymbolServerConverter {
   // store to try.  The vector must contain at least one string.  None of the
   // strings passed to this constructor may contain asterisk ('*') or semicolon
   // (';') characters, as the symbol engine uses these characters as separators.
+  // If |trace_symsrv| is set then callbacks from SymSrv will be logged to
+  // stderr.
   MSSymbolServerConverter(const string& local_cache,
-                          const vector<string>& symbol_servers);
+                          const vector<string>& symbol_servers,
+                          bool trace_symsrv);
 
   // Locates the PE file (DLL or EXE) specified by the identifying information
   // in |missing|, by checking the symbol stores identified when the object
@@ -226,8 +230,12 @@ class MSSymbolServerConverter {
   // SymFindFileInPath fails for an expected reason.
   bool fail_dns_;        // DNS failures (fail_not_found_ will also be set).
   bool fail_timeout_;    // Timeouts (fail_not_found_ will also be set).
+  bool fail_http_https_redir_;  // Bad URL-- we should be using HTTPS.
   bool fail_not_found_;  // The file could not be found.  If this is the only
                          // fail_* member set, then it is authoritative.
+
+  // If set then callbacks from SymSrv will be logged to stderr.
+  bool trace_symsrv_;
 };
 
 }  // namespace google_breakpad

src/tools/windows/converter_exe/converter.cc

@@ -303,9 +303,7 @@ static bool SafeToMakeExternalRequest(const MissingSymbolInfo& missing_info,
 
 // Converter options derived from command line parameters.
 struct ConverterOptions {
-  ConverterOptions()
+  ConverterOptions() : report_fetch_failures(true), trace_symsrv(false) {}
-      : report_fetch_failures(true) {
-  }
 
   ~ConverterOptions() {
   }
@@ -352,6 +350,9 @@ struct ConverterOptions {
   // Owned and cleaned up by this struct.
   std::regex blacklist_regex;
 
+  // If set then SymSrv callbacks are logged to stderr.
+  bool trace_symsrv;
+
  private:
   // DISABLE_COPY_AND_ASSIGN
   ConverterOptions(const ConverterOptions&);
@@ -416,7 +417,8 @@ static void ConvertMissingSymbolFile(const MissingSymbolInfo& missing_info,
     FprintfFlush(stderr, "Making internal request for %s (%s)\n",
                    missing_info.debug_file.c_str(),
                    missing_info.debug_identifier.c_str());
-    MSSymbolServerConverter converter(options.local_cache_path, msss_servers);
+    MSSymbolServerConverter converter(options.local_cache_path, msss_servers,
+                                      options.trace_symsrv);
     located = converter.LocateAndConvertSymbolFile(
         missing_info,
         /*keep_symbol_file=*/true,
@@ -470,6 +472,16 @@ static void ConvertMissingSymbolFile(const MissingSymbolInfo& missing_info,
         // a record.
         break;
 
+      case MSSymbolServerConverter::LOCATE_HTTP_HTTPS_REDIR:
+        FprintfFlush(
+            stderr,
+            "LocateResult = LOCATE_HTTP_HTTPS_REDIR\n"
+            "One of the specified URLs is using HTTP, which causes a redirect "
+            "from the server to HTTPS, which causes the SymSrv lookup to "
+            "fail.\n"
+            "This URL must be replaced with the correct HTTPS URL.\n");
+        break;
+
       case MSSymbolServerConverter::LOCATE_FAILURE:
         FprintfFlush(stderr, "LocateResult = LOCATE_FAILURE\n");
         // LocateAndConvertSymbolFile printed an error message.
@@ -503,8 +515,8 @@ static void ConvertMissingSymbolFile(const MissingSymbolInfo& missing_info,
       FprintfFlush(stderr, "Making external request for %s (%s)\n",
                    missing_info.debug_file.c_str(),
                    missing_info.debug_identifier.c_str());
-      MSSymbolServerConverter external_converter(options.local_cache_path,
+      MSSymbolServerConverter external_converter(
-                                                 msss_servers);
+          options.local_cache_path, msss_servers, options.trace_symsrv);
       located = external_converter.LocateAndConvertSymbolFile(
           missing_info,
           /*keep_symbol_file=*/true,
@@ -577,6 +589,15 @@ static void ConvertMissingSymbolFile(const MissingSymbolInfo& missing_info,
                    missing_info.version.c_str());
       break;
 
+    case MSSymbolServerConverter::LOCATE_HTTP_HTTPS_REDIR:
+      FprintfFlush(
+          stderr,
+          "LocateResult = LOCATE_HTTP_HTTPS_REDIR\n"
+          "One of the specified URLs is using HTTP, which causes a redirect "
+          "from the server to HTTPS, which causes the SymSrv lookup to fail.\n"
+          "This URL must be replaced with the correct HTTPS URL.\n");
+      break;
+
     case MSSymbolServerConverter::LOCATE_FAILURE:
       FprintfFlush(stderr, "LocateResult = LOCATE_FAILURE\n");
       // LocateAndConvertSymbolFile printed an error message.
@@ -708,6 +729,8 @@ static int usage(const char* program_name) {
       "    -t  <fetch_failure_url>    URL to report symbol fetch failure\n"
       "    -b  <regex>                Regex used to blacklist files to\n"
       "                               prevent external symbol requests\n"
+      "    -tss                       If set then SymSrv callbacks will be\n"
+      "                               traced to stderr.\n"
       " Note that any server specified by -f or -n that starts with \\filer\n"
       " will be treated as internal, and all others as external.\n",
       program_name);
@@ -794,6 +817,9 @@ int main(int argc, char** argv) {
       printf("Getting the list of missing symbols from a file.  Fetch failures"
              " will not be reported.\n");
       options.report_fetch_failures = false;
+    } else if (option == "-tss") {
+      printf("Tracing SymSrv callbacks to stderr.\n");
+      options.trace_symsrv = true;
     } else if (option == "-t") {
       if (!WindowsStringUtils::safe_mbstowcs(
           value,