From 9ce4475907ca19997141d450d465cfd5ed74ddce Mon Sep 17 00:00:00 2001
From: asynts <asynts@gmail.com>
Date: Mon, 31 Aug 2020 13:12:15 +0200
Subject: [PATCH] Streams: Distinguish recoverable and fatal errors.

---
 AK/BitStream.h                           |  4 +-
 AK/CircularDuplexStream.h                | 12 +++---
 AK/Stream.h                              | 47 +++++++++++++++++-------
 AK/String.h                              |  3 +-
 AK/Tests/TestStream.cpp                  | 20 +++++-----
 Libraries/LibCompress/Deflate.cpp        | 18 ++++-----
 Libraries/LibCompress/Gzip.cpp           | 10 ++---
 Libraries/LibCore/FileStream.h           | 14 +++----
 Libraries/LibDebug/Dwarf/LineProgram.cpp |  6 +--
 9 files changed, 75 insertions(+), 59 deletions(-)

diff --git a/AK/BitStream.h b/AK/BitStream.h
index 83bf9fcaa8..8684a349c2 100644
--- a/AK/BitStream.h
+++ b/AK/BitStream.h
@@ -57,7 +57,7 @@ public:
     bool read_or_error(Bytes bytes) override
     {
         if (read(bytes) != bytes.size()) {
-            m_error = true;
+            set_fatal_error();
             return false;
         }
 
@@ -92,7 +92,7 @@ public:
                 if (m_bit_offset++ == 7)
                     m_next_byte.clear();
             } else if (m_stream.eof()) {
-                m_error = true;
+                set_fatal_error();
                 return 0;
             } else {
                 m_stream >> m_next_byte;
diff --git a/AK/CircularDuplexStream.h b/AK/CircularDuplexStream.h
index 170490418b..bc2d472825 100644
--- a/AK/CircularDuplexStream.h
+++ b/AK/CircularDuplexStream.h
@@ -50,7 +50,7 @@ public:
     bool write_or_error(ReadonlyBytes bytes) override
     {
         if (Capacity - m_queue.size() < bytes.size()) {
-            m_error = true;
+            set_recoverable_error();
             return false;
         }
 
@@ -70,10 +70,8 @@ public:
 
     size_t read(Bytes bytes, size_t seekback)
     {
-        ASSERT(seekback <= Capacity);
-
-        if (seekback > m_total_written) {
-            m_error = true;
+        if (seekback > Capacity || seekback > m_total_written) {
+            set_recoverable_error();
             return 0;
         }
 
@@ -90,7 +88,7 @@ public:
     bool read_or_error(Bytes bytes) override
     {
         if (m_queue.size() < bytes.size()) {
-            m_error = true;
+            set_recoverable_error();
             return false;
         }
 
@@ -101,7 +99,7 @@ public:
     bool discard_or_error(size_t count) override
     {
         if (m_queue.size() < count) {
-            m_error = true;
+            set_recoverable_error();
             return false;
         }
 
diff --git a/AK/Stream.h b/AK/Stream.h
index 5b803d2367..2adf76a2d4 100644
--- a/AK/Stream.h
+++ b/AK/Stream.h
@@ -40,17 +40,36 @@ namespace AK::Detail {
 
 class Stream {
 public:
-    virtual ~Stream()
+    virtual ~Stream() { ASSERT(!has_any_error()); }
+
+    bool has_recoverable_error() const { return m_recoverable_error; }
+    bool has_fatal_error() const { return m_fatal_error; }
+    bool has_any_error() const { return has_recoverable_error() || has_fatal_error(); }
+
+    bool handle_recoverable_error()
     {
-        ASSERT(!has_error());
+        ASSERT(!has_fatal_error());
+        return exchange(m_recoverable_error, false);
+    }
+    bool handle_fatal_error() { return exchange(m_fatal_error, false); }
+    bool handle_any_error()
+    {
+        if (has_any_error()) {
+            m_recoverable_error = false;
+            m_fatal_error = false;
+
+            return true;
+        }
+
+        return false;
     }
 
-    bool has_error() const { return m_error; }
+    void set_recoverable_error() const { m_recoverable_error = true; }
+    void set_fatal_error() const { m_fatal_error = true; }
 
-    bool handle_error() { return exchange(m_error, false); }
-
-protected:
-    mutable bool m_error { false };
+private:
+    mutable bool m_recoverable_error { false };
+    mutable bool m_fatal_error { false };
 };
 
 }
@@ -200,7 +219,7 @@ public:
     bool read_or_error(Bytes bytes) override
     {
         if (remaining() < bytes.size()) {
-            m_error = true;
+            set_recoverable_error();
             return false;
         }
 
@@ -212,7 +231,7 @@ public:
     bool discard_or_error(size_t count) override
     {
         if (remaining() < count) {
-            m_error = true;
+            set_recoverable_error();
             return false;
         }
 
@@ -229,7 +248,7 @@ public:
     u8 peek_or_error() const
     {
         if (remaining() == 0) {
-            m_error = true;
+            set_recoverable_error();
             return 0;
         }
 
@@ -248,7 +267,7 @@ public:
             //       past the end, this is fixed here.
             if (eof()) {
                 m_offset = backup;
-                m_error = true;
+                set_recoverable_error();
                 return false;
             }
 
@@ -277,7 +296,7 @@ public:
             //       past the end, this is fixed here.
             if (eof()) {
                 m_offset = backup;
-                m_error = true;
+                set_recoverable_error();
                 return false;
             }
 
@@ -320,7 +339,7 @@ public:
     bool discard_or_error(size_t count) override
     {
         if (m_write_offset - m_read_offset < count) {
-            m_error = true;
+            set_recoverable_error();
             return false;
         }
 
@@ -411,7 +430,7 @@ public:
     bool read_or_error(Bytes bytes) override
     {
         if (m_write_offset - m_read_offset < bytes.size()) {
-            m_error = true;
+            set_recoverable_error();
             return false;
         }
 
diff --git a/AK/String.h b/AK/String.h
index 1ddbd9b40e..4e7b84f72f 100644
--- a/AK/String.h
+++ b/AK/String.h
@@ -288,8 +288,7 @@ inline InputStream& operator>>(InputStream& stream, String& string)
         if (stream.eof()) {
             string = nullptr;
 
-            // FIXME: We need an InputStream::set_error_flag method.
-            stream.discard_or_error(1);
+            stream.set_fatal_error();
             return stream;
         }
 
diff --git a/AK/Tests/TestStream.cpp b/AK/Tests/TestStream.cpp
index 5a1e6681d1..085631bb09 100644
--- a/AK/Tests/TestStream.cpp
+++ b/AK/Tests/TestStream.cpp
@@ -49,7 +49,7 @@ TEST_CASE(read_an_integer)
     InputMemoryStream stream { { &expected, sizeof(expected) } };
     stream >> actual;
 
-    EXPECT(!stream.has_error() && stream.eof());
+    EXPECT(!stream.has_any_error() && stream.eof());
     EXPECT_EQ(expected, actual);
 }
 
@@ -60,15 +60,15 @@ TEST_CASE(recoverable_error)
 
     InputMemoryStream stream { { &expected, sizeof(expected) } };
 
-    EXPECT(!stream.has_error() && !stream.eof());
+    EXPECT(!stream.has_any_error() && !stream.eof());
     stream >> to_large_value;
-    EXPECT(stream.has_error() && !stream.eof());
+    EXPECT(stream.has_recoverable_error() && !stream.eof());
 
-    EXPECT(stream.handle_error());
-    EXPECT(!stream.has_error() && !stream.eof());
+    EXPECT(stream.handle_recoverable_error());
+    EXPECT(!stream.has_any_error() && !stream.eof());
 
     stream >> actual;
-    EXPECT(!stream.has_error() && stream.eof());
+    EXPECT(!stream.has_any_error() && stream.eof());
     EXPECT_EQ(expected, actual);
 }
 
@@ -79,7 +79,7 @@ TEST_CASE(chain_stream_operator)
     InputMemoryStream stream { { expected, sizeof(expected) } };
 
     stream >> actual[0] >> actual[1] >> actual[2] >> actual[3];
-    EXPECT(!stream.has_error() && stream.eof());
+    EXPECT(!stream.has_any_error() && stream.eof());
 
     EXPECT(compare({ expected, sizeof(expected) }, { actual, sizeof(actual) }));
 }
@@ -95,17 +95,17 @@ TEST_CASE(seeking_slicing_offset)
     InputMemoryStream stream { { input, sizeof(input) } };
 
     stream >> Bytes { actual0, sizeof(actual0) };
-    EXPECT(!stream.has_error() && !stream.eof());
+    EXPECT(!stream.has_any_error() && !stream.eof());
     EXPECT(compare({ expected0, sizeof(expected0) }, { actual0, sizeof(actual0) }));
 
     stream.seek(4);
     stream >> Bytes { actual1, sizeof(actual1) };
-    EXPECT(!stream.has_error() && stream.eof());
+    EXPECT(!stream.has_any_error() && stream.eof());
     EXPECT(compare({ expected1, sizeof(expected1) }, { actual1, sizeof(actual1) }));
 
     stream.seek(1);
     stream >> Bytes { actual2, sizeof(actual2) };
-    EXPECT(!stream.has_error() && !stream.eof());
+    EXPECT(!stream.has_any_error() && !stream.eof());
     EXPECT(compare({ expected2, sizeof(expected2) }, { actual2, sizeof(actual2) }));
 }
 
diff --git a/Libraries/LibCompress/Deflate.cpp b/Libraries/LibCompress/Deflate.cpp
index a7f7757d66..81ddb6cb93 100644
--- a/Libraries/LibCompress/Deflate.cpp
+++ b/Libraries/LibCompress/Deflate.cpp
@@ -204,7 +204,7 @@ size_t DeflateDecompressor::read(Bytes bytes)
             m_input_stream >> length >> negated_length;
 
             if ((length ^ 0xffff) != negated_length) {
-                m_error = true;
+                set_fatal_error();
                 return 0;
             }
 
@@ -273,7 +273,7 @@ size_t DeflateDecompressor::read(Bytes bytes)
 bool DeflateDecompressor::read_or_error(Bytes bytes)
 {
     if (read(bytes) < bytes.size()) {
-        m_error = true;
+        set_fatal_error();
         return false;
     }
 
@@ -287,7 +287,7 @@ bool DeflateDecompressor::discard_or_error(size_t count)
     size_t ndiscarded = 0;
     while (ndiscarded < count) {
         if (eof()) {
-            m_error = true;
+            set_fatal_error();
             return false;
         }
 
@@ -371,7 +371,7 @@ void DeflateDecompressor::decode_codes(CanonicalCode& literal_code, Optional<Can
 
     auto code_length_code_result = CanonicalCode::from_bytes({ code_lengths_code_lengths, sizeof(code_lengths_code_lengths) });
     if (!code_length_code_result.has_value()) {
-        m_error = true;
+        set_fatal_error();
         return;
     }
     const auto code_length_code = code_length_code_result.value();
@@ -399,7 +399,7 @@ void DeflateDecompressor::decode_codes(CanonicalCode& literal_code, Optional<Can
             ASSERT(symbol == 16);
 
             if (code_lengths.is_empty()) {
-                m_error = true;
+                set_fatal_error();
                 return;
             }
 
@@ -410,7 +410,7 @@ void DeflateDecompressor::decode_codes(CanonicalCode& literal_code, Optional<Can
     }
 
     if (code_lengths.size() != literal_code_count + distance_code_count) {
-        m_error = true;
+        set_fatal_error();
         return;
     }
 
@@ -418,7 +418,7 @@ void DeflateDecompressor::decode_codes(CanonicalCode& literal_code, Optional<Can
 
     auto literal_code_result = CanonicalCode::from_bytes(code_lengths.span().trim(literal_code_count));
     if (!literal_code_result.has_value()) {
-        m_error = true;
+        set_fatal_error();
         return;
     }
     literal_code = literal_code_result.value();
@@ -431,14 +431,14 @@ void DeflateDecompressor::decode_codes(CanonicalCode& literal_code, Optional<Can
         if (length == 0) {
             return;
         } else if (length != 1) {
-            m_error = true;
+            set_fatal_error();
             return;
         }
     }
 
     auto distance_code_result = CanonicalCode::from_bytes(code_lengths.span().slice(literal_code_count));
     if (!distance_code_result.has_value()) {
-        m_error = true;
+        set_fatal_error();
         return;
     }
     distance_code = distance_code_result.value();
diff --git a/Libraries/LibCompress/Gzip.cpp b/Libraries/LibCompress/Gzip.cpp
index 3e7170342c..56d2a5f9b7 100644
--- a/Libraries/LibCompress/Gzip.cpp
+++ b/Libraries/LibCompress/Gzip.cpp
@@ -77,12 +77,12 @@ size_t GzipDecompressor::read(Bytes bytes)
             m_input_stream >> crc32 >> input_size;
 
             if (crc32 != current_member().m_checksum.digest()) {
-                m_error = true;
+                set_fatal_error();
                 return 0;
             }
 
             if (input_size != current_member().m_nread) {
-                m_error = true;
+                set_fatal_error();
                 return 0;
             }
 
@@ -101,7 +101,7 @@ size_t GzipDecompressor::read(Bytes bytes)
         m_input_stream >> Bytes { &header, sizeof(header) };
 
         if (!header.valid_magic_number() || !header.supported_by_implementation()) {
-            m_error = true;
+            set_fatal_error();
             return 0;
         }
 
@@ -129,7 +129,7 @@ size_t GzipDecompressor::read(Bytes bytes)
 bool GzipDecompressor::read_or_error(Bytes bytes)
 {
     if (read(bytes) < bytes.size()) {
-        m_error = true;
+        set_fatal_error();
         return false;
     }
 
@@ -143,7 +143,7 @@ bool GzipDecompressor::discard_or_error(size_t count)
     size_t ndiscarded = 0;
     while (ndiscarded < count) {
         if (eof()) {
-            m_error = true;
+            set_fatal_error();
             return false;
         }
 
diff --git a/Libraries/LibCore/FileStream.h b/Libraries/LibCore/FileStream.h
index eb0d6df9d2..43c8475c61 100644
--- a/Libraries/LibCore/FileStream.h
+++ b/Libraries/LibCore/FileStream.h
@@ -63,7 +63,7 @@ public:
 
         while (nread < bytes.size() && !eof()) {
             if (m_file->has_error()) {
-                m_error = true;
+                set_fatal_error();
                 return 0;
             }
 
@@ -77,7 +77,7 @@ public:
     bool read_or_error(Bytes bytes) override
     {
         if (read(bytes) < bytes.size()) {
-            m_error = true;
+            set_fatal_error();
             return false;
         }
 
@@ -93,7 +93,7 @@ public:
             ndiscarded += read({ buffer, min<size_t>(count - ndiscarded, sizeof(buffer)) });
 
         if (eof()) {
-            m_error = true;
+            set_fatal_error();
             return false;
         }
 
@@ -116,7 +116,7 @@ public:
     void close()
     {
         if (!m_file->close())
-            m_error = true;
+            set_fatal_error();
     }
 
 private:
@@ -153,7 +153,7 @@ public:
     size_t write(ReadonlyBytes bytes) override
     {
         if (!m_file->write(bytes.data(), bytes.size())) {
-            m_error = true;
+            set_fatal_error();
             return 0;
         }
 
@@ -163,7 +163,7 @@ public:
     bool write_or_error(ReadonlyBytes bytes) override
     {
         if (write(bytes) < bytes.size()) {
-            m_error = true;
+            set_fatal_error();
             return false;
         }
 
@@ -173,7 +173,7 @@ public:
     void close()
     {
         if (!m_file->close())
-            m_error = true;
+            set_fatal_error();
     }
 
 private:
diff --git a/Libraries/LibDebug/Dwarf/LineProgram.cpp b/Libraries/LibDebug/Dwarf/LineProgram.cpp
index a47b174a4b..b2f23700b0 100644
--- a/Libraries/LibDebug/Dwarf/LineProgram.cpp
+++ b/Libraries/LibDebug/Dwarf/LineProgram.cpp
@@ -66,9 +66,9 @@ void LineProgram::parse_source_directories()
 #endif
         m_source_directories.append(move(directory));
     }
-    m_stream.handle_error();
+    m_stream.handle_recoverable_error();
     m_stream.discard_or_error(1);
-    ASSERT(!m_stream.handle_error());
+    ASSERT(!m_stream.has_any_error());
 }
 
 void LineProgram::parse_source_files()
@@ -88,7 +88,7 @@ void LineProgram::parse_source_files()
         m_source_files.append({ file_name, directory_index });
     }
     m_stream.discard_or_error(1);
-    ASSERT(!m_stream.handle_error());
+    ASSERT(!m_stream.has_any_error());
 }
 
 void LineProgram::append_to_line_info()