From 737e2cd338d68e25d6757afdd2822ac953f83a7e Mon Sep 17 00:00:00 2001 From: Nelson Billing Date: Fri, 3 Jun 2022 15:47:48 -0700 Subject: [PATCH] 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 --- .../converter/ms_symbol_server_converter.cc | 59 ++++++++++++------- .../converter/ms_symbol_server_converter.h | 16 +++-- src/tools/windows/converter_exe/converter.cc | 38 ++++++++++-- 3 files changed, 81 insertions(+), 32 deletions(-) diff --git a/src/tools/windows/converter/ms_symbol_server_converter.cc b/src/tools/windows/converter/ms_symbol_server_converter.cc index 102d06fd..4645644a 100644 --- a/src/tools/windows/converter/ms_symbol_server_converter.cc +++ b/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& symbol_servers) + const string& local_cache, + const vector& 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. @@ -434,29 +446,32 @@ 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", - &self->fail_dns_ }, + // When a DNS error occurs, it could be indiciative of network + // problems. + {"SYMSRV: The server name or address could not be resolved\n", + &self->fail_dns_}, - // This message is produced if no connection is opened. - { "SYMSRV: A connection with the server could not be established\n", - &self->fail_timeout_ }, + // This message is produced if no connection is opened. + {"SYMSRV: A connection with the server could not be established\n", + &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", - &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", &self->fail_timeout_}, - // This message is produced when the requested file is not found, - // even if one or more of the above messages are also produced. - // It's trapped to distinguish between not-found and unknown-failure - // 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", - &self->fail_not_found_ } - }; + // 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. + // It's trapped to distinguish between not-found and unknown-failure + // 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", &self->fail_not_found_}}; for (int desc_action_index = 0; desc_action_index < diff --git a/src/tools/windows/converter/ms_symbol_server_converter.h b/src/tools/windows/converter/ms_symbol_server_converter.h index 40d65d52..a41cad16 100644 --- a/src/tools/windows/converter/ms_symbol_server_converter.h +++ b/src/tools/windows/converter/ms_symbol_server_converter.h @@ -127,9 +127,10 @@ class MSSymbolServerConverter { public: enum LocateResult { LOCATE_FAILURE = 0, - LOCATE_NOT_FOUND, // Authoritative: the file is not present. - LOCATE_RETRY, // Transient (network?) error, try again later. - LOCATE_SUCCESS + LOCATE_NOT_FOUND, // Authoritative: the file is not present. + LOCATE_RETRY, // Transient (network?) error, try again later. + 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& symbol_servers); + const vector& 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 diff --git a/src/tools/windows/converter_exe/converter.cc b/src/tools/windows/converter_exe/converter.cc index 3e0d35d2..bb0d091e 100644 --- a/src/tools/windows/converter_exe/converter.cc +++ b/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() - : report_fetch_failures(true) { - } + ConverterOptions() : report_fetch_failures(true), trace_symsrv(false) {} ~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, - msss_servers); + MSSymbolServerConverter external_converter( + 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 URL to report symbol fetch failure\n" " -b 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,