Stefan =?utf-8?q?Gränitz?= <[email protected]> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/[email protected]>
llvmbot wrote: <!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb @llvm/pr-subscribers-llvm-support Author: Stefan Gränitz (weliveindetail) <details> <summary>Changes</summary> SymbolLocatorSymStore used a simple local implementation of HTTPResponseHandler so far. That was fine for basic usage, but it would cause issues down the line. This patch hoists the StreamedHTTPResponseHandler class from libDebuginfod to SupportHTTP and integrates it in SymbolLocatorSymStore. PDB file downloads will now be buffered on disk, which is necessary since they can be huge. We use the opportunity an stop logging 404 responses (file not found on server) and print warnings for all other erroneous HTTP responses. It was more complicated before, because the old response handler created the underlying file in any case. The new one does that only one the first content package comes in. --- Full diff: https://github.com/llvm/llvm-project/pull/187687.diff 6 Files Affected: - (modified) lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp (+51-54) - (modified) lldb/test/API/symstore/TestSymStore.py (+11) - (added) llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h (+49) - (modified) llvm/lib/Debuginfod/Debuginfod.cpp (+1-47) - (modified) llvm/lib/Support/HTTP/CMakeLists.txt (+1) - (added) llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp (+34) ``````````diff diff --git a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp index 884519b54acfd..02c568816f706 100644 --- a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp +++ b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp @@ -18,11 +18,14 @@ #include "lldb/Utility/UUID.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/Support/Caching.h" #include "llvm/Support/Endian.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/HTTP/HTTPClient.h" +#include "llvm/Support/HTTP/StreamedHTTPResponseHandler.h" #include "llvm/Support/Path.h" +#include "llvm/Support/raw_ostream.h" using namespace lldb; using namespace lldb_private; @@ -114,56 +117,6 @@ std::string formatSymStoreKey(const UUID &uuid) { return llvm::toHex(bytes.slice(0, 16), LowerCase) + std::to_string(age); } -// This is a simple version of Debuginfod's StreamedHTTPResponseHandler. We -// should consider reusing that once we introduce caching. -class FileDownloadHandler : public llvm::HTTPResponseHandler { -private: - std::error_code m_ec; - llvm::raw_fd_ostream m_stream; - -public: - FileDownloadHandler(llvm::StringRef file) : m_stream(file.str(), m_ec) {} - virtual ~FileDownloadHandler() = default; - - llvm::Error handleBodyChunk(llvm::StringRef data) override { - // Propagate error from ctor. - if (m_ec) - return llvm::createStringError(m_ec, "Failed to open file for writing"); - m_stream.write(data.data(), data.size()); - if (std::error_code ec = m_stream.error()) - return llvm::createStringError(ec, "Error writing to file"); - - return llvm::Error::success(); - } -}; - -llvm::Error downloadFileHTTP(llvm::StringRef url, FileDownloadHandler dest) { - if (!llvm::HTTPClient::isAvailable()) - return llvm::createStringError( - std::make_error_code(std::errc::not_supported), - "HTTP client is not available"); - llvm::HTTPRequest Request(url); - Request.FollowRedirects = true; - - llvm::HTTPClient Client; - - // TODO: Since PDBs can be huge, we should distinguish between resolve, - // connect, send and receive. - Client.setTimeout(std::chrono::seconds(60)); - - if (llvm::Error Err = Client.perform(Request, dest)) - return Err; - - unsigned ResponseCode = Client.responseCode(); - if (ResponseCode != 200) { - return llvm::createStringError(std::make_error_code(std::errc::io_error), - "HTTP request failed with status code " + - std::to_string(ResponseCode)); - } - - return llvm::Error::success(); -} - bool has_unsafe_characters(llvm::StringRef s) { for (unsigned char c : s) { // RFC 3986 unreserved characters are safe for file names and URLs. @@ -216,13 +169,57 @@ requestFileFromSymStoreServerHTTP(llvm::StringRef base_url, llvm::StringRef key, // Server has same directory structure with forward slashes as separators. std::string source_url = llvm::formatv("{0}/{1}/{2}/{1}", base_url, pdb_name, key); - if (llvm::Error err = downloadFileHTTP(source_url, cache_file.str())) { - LLDB_LOG_ERROR(log, std::move(err), - "Failed to download from SymStore '{1}': {0}", source_url); + + if (!llvm::HTTPClient::isAvailable()) { + Debugger::ReportWarning("HTTP client is not available"); return {}; } - return FileSpec(cache_file.str()); + llvm::HTTPClient Client; + // TODO: Since PDBs can be huge, we should distinguish between resolve, + // connect, send and receive. + Client.setTimeout(std::chrono::seconds(60)); + + llvm::StreamedHTTPResponseHandler Handler( + [dest = cache_file.str().str()]() + -> llvm::Expected<std::unique_ptr<llvm::CachedFileStream>> { + std::error_code EC; + auto FDStream = std::make_unique<llvm::raw_fd_ostream>(dest, EC); + if (EC) + return llvm::createStringError(EC, "Failed to open file for writing"); + return std::make_unique<llvm::CachedFileStream>(std::move(FDStream), + dest); + }, + Client); + + llvm::HTTPRequest Request(source_url); + Request.FollowRedirects = true; + + if (llvm::Error Err = Client.perform(Request, Handler)) { + Debugger::ReportWarning( + llvm::formatv("Failed to download from SymStore '{0}': {1}", source_url, + llvm::toString(std::move(Err)))); + return {}; + } + if (llvm::Error Err = Handler.commit()) { + Debugger::ReportWarning( + llvm::formatv("Failed to download from SymStore '{0}': {1}", source_url, + llvm::toString(std::move(Err)))); + return {}; + } + + unsigned ResponseCode = Client.responseCode(); + switch (ResponseCode) { + case 404: + return {}; // file not found + case 200: + return FileSpec(cache_file.str()); // success + default: + Debugger::ReportWarning(llvm::formatv( + "Failed to download from SymStore '{0}': response code {1}", source_url, + ResponseCode)); + return {}; + } } std::optional<FileSpec> findFileInLocalSymStore(llvm::StringRef root_dir, diff --git a/lldb/test/API/symstore/TestSymStore.py b/lldb/test/API/symstore/TestSymStore.py index 13d0cc1666c84..63f7f10a13de7 100644 --- a/lldb/test/API/symstore/TestSymStore.py +++ b/lldb/test/API/symstore/TestSymStore.py @@ -140,6 +140,17 @@ def test_local_dir(self): self.runCmd(f"settings set plugin.symbol-locator.symstore.urls {dir}") self.try_breakpoint(exe, should_have_loc=True) + def test_http_not_found(self): + """ + Check that a 404 response from an HTTP SymStore is handled gracefully. + """ + exe, sym = self.build_inferior() + with MockedSymStore(self, exe, sym) as symstore_dir: + os.makedirs(f"{symstore_dir}_empty", exist_ok=False) + with HTTPServer(f"{symstore_dir}_empty") as url: + self.runCmd(f"settings set plugin.symbol-locator.symstore.urls {url}") + self.try_breakpoint(exe, should_have_loc=False) + # TODO: Add test coverage for common HTTPS security scenarios, e.g. self-signed # certs, non-HTTPS redirects, etc. def test_http(self): diff --git a/llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h b/llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h new file mode 100644 index 0000000000000..14ffab8aa1be3 --- /dev/null +++ b/llvm/include/llvm/Support/HTTP/StreamedHTTPResponseHandler.h @@ -0,0 +1,49 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// An HTTPResponseHandler that streams the response body to a CachedFileStream. +/// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_SUPPORT_HTTP_STREAMEDHTTPRESPONSEHANDLER_H +#define LLVM_SUPPORT_HTTP_STREAMEDHTTPRESPONSEHANDLER_H + +#include "llvm/Support/Caching.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/HTTP/HTTPClient.h" +#include <functional> +#include <memory> + +namespace llvm { + +/// A handler which streams the returned data to a CachedFileStream. The cache +/// file is only created if a 200 OK status is observed. +class StreamedHTTPResponseHandler : public HTTPResponseHandler { + using CreateStreamFn = + std::function<Expected<std::unique_ptr<CachedFileStream>>()>; + CreateStreamFn CreateStream; + HTTPClient &Client; + std::unique_ptr<CachedFileStream> FileStream; + +public: + StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client) + : CreateStream(std::move(CreateStream)), Client(Client) {} + + /// Must be called exactly once after the writes have been completed + /// but before the StreamedHTTPResponseHandler object is destroyed. + Error commit(); + + virtual ~StreamedHTTPResponseHandler() = default; + + Error handleBodyChunk(StringRef BodyChunk) override; +}; + +} // end namespace llvm + +#endif // LLVM_SUPPORT_HTTP_STREAMEDHTTPRESPONSEHANDLER_H diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp index 6847dc092dfdb..77de51d90a4eb 100644 --- a/llvm/lib/Debuginfod/Debuginfod.cpp +++ b/llvm/lib/Debuginfod/Debuginfod.cpp @@ -35,6 +35,7 @@ #include "llvm/Support/Error.h" #include "llvm/Support/FileUtilities.h" #include "llvm/Support/HTTP/HTTPClient.h" +#include "llvm/Support/HTTP/StreamedHTTPResponseHandler.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/Path.h" #include "llvm/Support/ThreadPool.h" @@ -174,53 +175,6 @@ Expected<std::string> getCachedOrDownloadArtifact(StringRef UniqueKey, getDefaultDebuginfodTimeout()); } -namespace { - -/// A simple handler which streams the returned data to a cache file. The cache -/// file is only created if a 200 OK status is observed. -class StreamedHTTPResponseHandler : public HTTPResponseHandler { - using CreateStreamFn = - std::function<Expected<std::unique_ptr<CachedFileStream>>()>; - CreateStreamFn CreateStream; - HTTPClient &Client; - std::unique_ptr<CachedFileStream> FileStream; - -public: - StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client) - : CreateStream(CreateStream), Client(Client) {} - - /// Must be called exactly once after the writes have been completed - /// but before the StreamedHTTPResponseHandler object is destroyed. - Error commit(); - - virtual ~StreamedHTTPResponseHandler() = default; - - Error handleBodyChunk(StringRef BodyChunk) override; -}; - -} // namespace - -Error StreamedHTTPResponseHandler::handleBodyChunk(StringRef BodyChunk) { - if (!FileStream) { - unsigned Code = Client.responseCode(); - if (Code && Code != 200) - return Error::success(); - Expected<std::unique_ptr<CachedFileStream>> FileStreamOrError = - CreateStream(); - if (!FileStreamOrError) - return FileStreamOrError.takeError(); - FileStream = std::move(*FileStreamOrError); - } - *FileStream->OS << BodyChunk; - return Error::success(); -} - -Error StreamedHTTPResponseHandler::commit() { - if (FileStream) - return FileStream->commit(); - return Error::success(); -} - // An over-accepting simplification of the HTTP RFC 7230 spec. static bool isHeader(StringRef S) { StringRef Name; diff --git a/llvm/lib/Support/HTTP/CMakeLists.txt b/llvm/lib/Support/HTTP/CMakeLists.txt index e7a0e6fe34110..f8e4b87539931 100644 --- a/llvm/lib/Support/HTTP/CMakeLists.txt +++ b/llvm/lib/Support/HTTP/CMakeLists.txt @@ -16,6 +16,7 @@ endif() add_llvm_component_library(LLVMSupportHTTP HTTPClient.cpp HTTPServer.cpp + StreamedHTTPResponseHandler.cpp LINK_LIBS ${imported_libs} diff --git a/llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp b/llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp new file mode 100644 index 0000000000000..5a0e0e6355684 --- /dev/null +++ b/llvm/lib/Support/HTTP/StreamedHTTPResponseHandler.cpp @@ -0,0 +1,34 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "llvm/Support/HTTP/StreamedHTTPResponseHandler.h" + +namespace llvm { + +Error StreamedHTTPResponseHandler::handleBodyChunk(StringRef BodyChunk) { + if (!FileStream) { + unsigned Code = Client.responseCode(); + if (Code && Code != 200) + return Error::success(); + Expected<std::unique_ptr<CachedFileStream>> FileStreamOrError = + CreateStream(); + if (!FileStreamOrError) + return FileStreamOrError.takeError(); + FileStream = std::move(*FileStreamOrError); + } + *FileStream->OS << BodyChunk; + return Error::success(); +} + +Error StreamedHTTPResponseHandler::commit() { + if (FileStream) + return FileStream->commit(); + return Error::success(); +} + +} // namespace llvm `````````` </details> https://github.com/llvm/llvm-project/pull/187687 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
