Merge bitcoin/bitcoin#35828: util: Make LineReader consistently use string_view

dff44e4c8f util: LineReader - Drop support for raw std::byte spans (Hodlinator)
5d5cdcd79d util: Make LineReader consistently use string_views (Hodlinator)
e8eaa80ce2 util: LineReader - Don't include newline and acknowledge single-char \r (Hodlinator)

Pull request description:

  3 commits changing `LineReader`:
  * Avoid the duplicate check for `\n` happening inside `RemoveSuffixView()`. https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3333154138
  * Use `string_view` internally rather than 2 `span::iterator`s.
  * Stop accepting `span<byte>` inputs since internally and as outputs we treat them as strings.

  Found while reviewing #35182.

ACKs for top commit:
  achow101:
    ACK dff44e4c8f
  pinheadmz:
    ACK dff44e4c8f
  furszy:
    ACK dff44e4c8f

Tree-SHA512: f4108cdc3895cce879eb21538ae6b11e7af3322abeb7026b766ca25419c53e691921ba94b1bc99dd34696b9bd3abcbde404ea62fc0f1e75d292f256d75fbf154

src/httpserver.cpp

@@ -861,7 +861,7 @@ void HTTPServer::SocketHandlerConnected(const IOReadiness& io_readiness) const
         }
 
         if (recv_ready || err_ready) {
-            std::byte buf[0x10000]; // typical socket buffer is 8K-64K
+            char buf[0x10000]; // typical socket buffer is 8K-64K
 
             const ssize_t nrecv{WITH_LOCK(
                 client->m_sock_mutex,

src/httpserver.h

@@ -461,7 +461,7 @@ public:
      * we copy data from the socket buffer to the client object
      * and attempt to read HTTP requests from here.
      */
-    std::vector<std::byte> m_recv_buffer{};
+    std::string m_recv_buffer{};
 
     //! Requests from a client must be processed in the order in which
     //! they were received, blocking on a per-client basis. We won't

src/test/fuzz/http_request.cpp

@@ -25,7 +25,7 @@ FUZZ_TARGET(http_request)
     using util::LineReader;
 
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
-    const std::vector<std::byte> http_buffer{ConsumeRandomLengthByteVector<std::byte>(fuzzed_data_provider, 4096)};
+    const std::string http_buffer{fuzzed_data_provider.ConsumeRandomLengthString(4096)};
 
     HTTPRequest http_request;
     LineReader reader(http_buffer, MAX_HEADERS_SIZE);

src/torcontrol.cpp

@@ -149,7 +149,7 @@ bool TorControlConnection::ReceiveAndProcess()
 {
     if (!m_sock) return false;
 
-    std::byte buf[4096];
+    char buf[4096];
     ssize_t nread = m_sock->Recv(buf, sizeof(buf), MSG_DONTWAIT);
 
     if (nread < 0) {
@@ -179,7 +179,6 @@ bool TorControlConnection::ReceiveAndProcess()
 bool TorControlConnection::ProcessBuffer()
 {
     util::LineReader reader(m_recv_buffer, MAX_LINE_LENGTH);
-    auto start = reader.it;
 
     while (auto line = reader.ReadLine()) {
         if (m_message.lines.size() == MAX_LINE_COUNT) {
@@ -210,7 +209,7 @@ bool TorControlConnection::ProcessBuffer()
         }
     }
 
-    m_recv_buffer.erase(m_recv_buffer.begin(), m_recv_buffer.begin() + std::distance(start, reader.it));
+    m_recv_buffer.erase(m_recv_buffer.begin(), m_recv_buffer.begin() + reader.Consumed());
     return true;
 }
 

src/torcontrol.h

@@ -109,7 +109,7 @@ private:
     /** Response handlers */
     std::deque<ReplyHandlerCB> m_reply_handlers;
     /** Buffer for incoming data */
-    std::vector<std::byte> m_recv_buffer;
+    std::string m_recv_buffer;
     /** Process complete lines from the receive buffer */
     bool ProcessBuffer();
 };

src/util/string.cpp

@@ -17,42 +17,41 @@ void ReplaceAll(std::string& in_out, const std::string& search, const std::strin
     in_out = std::regex_replace(in_out, std::regex(search), substitute);
 }
 
-LineReader::LineReader(std::span<const std::byte> buffer, size_t max_line_length)
-    : start(buffer.begin()), end(buffer.end()), max_line_length(max_line_length), it(buffer.begin()) {}
+LineReader::LineReader(std::string_view str, size_t max_line_length)
+    : m_str{str}, m_max_line_length{max_line_length}, m_it{str.begin()} {}
 
 std::optional<std::string_view> LineReader::ReadLine()
 {
-    if (it == end) {
+    if (m_it == m_str.end()) {
         return std::nullopt;
     }
 
-    auto line_start = it;
-    size_t count = 0;
-    while (it != end) {
+    const auto line_start = m_it;
+    while (m_it != m_str.end()) {
         // Read a character from the incoming buffer and increment the iterator
-        auto c = static_cast<char>(*it);
-        ++it;
-        ++count;
+        const bool new_line{*m_it == '\n'};
+        ++m_it;
         // If the character we just consumed was \n, the line is terminated.
         // The \n itself does not count against max_line_length.
-        if (c == '\n') {
-            const std::string_view untrimmed_line(reinterpret_cast<const char*>(std::to_address(line_start)), count);
-            std::string_view line = RemoveSuffixView(untrimmed_line, "\n");
-            return RemoveSuffixView(line, "\r");
+        if (new_line) {
+            std::string_view line{line_start, m_it - 1};
+            if (!line.empty() && line.back() == '\r')
+                line.remove_suffix(1);
+            return line;
         }
         // If the character we just consumed gives us a line length greater
         // than max_line_length, and we are not at the end of the line (or buffer) yet,
         // that means the line we are currently reading is too long, and we throw.
-        if (count > max_line_length) {
+        if (static_cast<size_t>(std::distance(line_start, m_it)) > m_max_line_length) {
             // Reset iterator
-            it = line_start;
+            m_it = line_start;
             throw std::runtime_error("max_line_length exceeded by LineReader");
         }
     }
     // End of buffer reached without finding a \n or exceeding max_line_length.
     // Reset the iterator so the rest of the buffer can be read granularly
     // with ReadLength() and return null to indicate a line was not found.
-    it = line_start;
+    m_it = line_start;
     return std::nullopt;
 }
 
@@ -61,18 +60,18 @@ std::string_view LineReader::ReadLength(size_t len)
 {
     if (len == 0) return {};
     if (Remaining() < len) throw std::runtime_error("Not enough data in buffer");
-    std::string_view out(reinterpret_cast<const char*>(std::to_address(it)), len);
-    it += len;
+    std::string_view out(std::to_address(m_it), len);
+    m_it += len;
     return out;
 }
 
 size_t LineReader::Remaining() const
 {
-    return std::distance(it, end);
+    return std::distance(m_it, m_str.end());
 }
 
 size_t LineReader::Consumed() const
 {
-    return std::distance(start, it);
+    return std::distance(m_str.begin(), m_it);
 }
 } // namespace util

src/util/string.h

@@ -265,14 +265,14 @@ template <typename T1, size_t PREFIX_LEN>
            std::equal(std::begin(prefix), std::end(prefix), std::begin(obj));
 }
 
-struct LineReader {
-    const std::span<const std::byte>::iterator start;
-    const std::span<const std::byte>::iterator end;
-    const size_t max_line_length;
-    std::span<const std::byte>::iterator it;
+class LineReader
+{
+    const std::string_view m_str;
+    const size_t m_max_line_length;
+    std::string_view::iterator m_it;
 
-    explicit LineReader(std::span<const std::byte> buffer, size_t max_line_length);
-    explicit LineReader(std::string_view str, size_t max_line_length) : LineReader{std::as_bytes(std::span{str}), max_line_length} {}
+public:
+    explicit LineReader(std::string_view str, size_t max_line_length);
 
     /**
      * Returns a string from current iterator position up to (but not including) next \n