diff --git a/Meta/Lagom/Fuzzers/FuzzGzipDecompression.cpp b/Meta/Lagom/Fuzzers/FuzzGzipDecompression.cpp index 00cf7a8b35..580ef03e41 100644 --- a/Meta/Lagom/Fuzzers/FuzzGzipDecompression.cpp +++ b/Meta/Lagom/Fuzzers/FuzzGzipDecompression.cpp @@ -10,5 +10,5 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { auto result = Compress::GzipDecompressor::decompress_all(ReadonlyBytes { data, size }); - return result.has_value(); + return 0; } diff --git a/Tests/LibCompress/TestGzip.cpp b/Tests/LibCompress/TestGzip.cpp index 7ec312874c..90c0acf045 100644 --- a/Tests/LibCompress/TestGzip.cpp +++ b/Tests/LibCompress/TestGzip.cpp @@ -92,6 +92,6 @@ TEST_CASE(gzip_round_trip) auto compressed = Compress::GzipCompressor::compress_all(original); EXPECT(compressed.has_value()); auto uncompressed = Compress::GzipDecompressor::decompress_all(compressed.value()); - EXPECT(uncompressed.has_value()); + EXPECT(!uncompressed.is_error()); EXPECT(uncompressed.value() == original); } diff --git a/Userland/Libraries/LibCompress/Gzip.cpp b/Userland/Libraries/LibCompress/Gzip.cpp index 8c07562b17..88f5650c3c 100644 --- a/Userland/Libraries/LibCompress/Gzip.cpp +++ b/Userland/Libraries/LibCompress/Gzip.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace Compress { @@ -38,8 +39,8 @@ bool BlockHeader::supported_by_implementation() const return true; } -GzipDecompressor::GzipDecompressor(InputStream& stream) - : m_input_stream(stream) +GzipDecompressor::GzipDecompressor(NonnullOwnPtr stream) + : m_input_stream(move(stream)) { } @@ -48,12 +49,11 @@ GzipDecompressor::~GzipDecompressor() m_current_member.clear(); } -// FIXME: Again, there are surely a ton of bugs because the code doesn't check for read errors. -size_t GzipDecompressor::read(Bytes bytes) +ErrorOr GzipDecompressor::read(Bytes bytes) { size_t total_read = 0; while (total_read < bytes.size()) { - if (has_any_error() || m_eof) + if (is_eof()) break; auto slice = bytes.slice(total_read); @@ -63,26 +63,19 @@ size_t GzipDecompressor::read(Bytes bytes) current_member().m_checksum.update(slice.trim(nread)); current_member().m_nread += nread; - if (current_member().m_stream.handle_any_error()) { - set_fatal_error(); - break; - } + if (current_member().m_stream.handle_any_error()) + return Error::from_string_literal("Underlying DeflateDecompressor indicated an error"); if (nread < slice.size()) { LittleEndian crc32, input_size; - m_input_stream >> crc32 >> input_size; + TRY(m_input_stream->read(crc32.bytes())); + TRY(m_input_stream->read(input_size.bytes())); - if (crc32 != current_member().m_checksum.digest()) { - // FIXME: Somehow the checksum is incorrect? + if (crc32 != current_member().m_checksum.digest()) + return Error::from_string_literal("Stored CRC32 does not match the calculated CRC32 of the current member"); - set_fatal_error(); - break; - } - - if (input_size != current_member().m_nread) { - set_fatal_error(); - break; - } + if (input_size != current_member().m_nread) + return Error::from_string_literal("Input size does not match the number of read bytes"); m_current_member.clear(); @@ -93,12 +86,12 @@ size_t GzipDecompressor::read(Bytes bytes) total_read += nread; continue; } else { - m_partial_header_offset += m_input_stream.read(Bytes { m_partial_header, sizeof(BlockHeader) }.slice(m_partial_header_offset)); + auto current_partial_header_slice = Bytes { m_partial_header, sizeof(BlockHeader) }.slice(m_partial_header_offset); + auto current_partial_header_data = TRY(m_input_stream->read(current_partial_header_slice)); + m_partial_header_offset += current_partial_header_data.size(); - if (m_input_stream.handle_any_error() || m_input_stream.unreliable_eof()) { - m_eof = true; + if (is_eof()) break; - } if (m_partial_header_offset < sizeof(BlockHeader)) { break; // partial header read @@ -107,51 +100,45 @@ size_t GzipDecompressor::read(Bytes bytes) BlockHeader header = *(reinterpret_cast(m_partial_header)); - if (!header.valid_magic_number() || !header.supported_by_implementation()) { - set_fatal_error(); - break; - } + if (!header.valid_magic_number()) + return Error::from_string_literal("Header does not have a valid magic number"); + + if (!header.supported_by_implementation()) + return Error::from_string_literal("Header is not supported by implementation"); if (header.flags & Flags::FEXTRA) { LittleEndian subfield_id, length; - m_input_stream >> subfield_id >> length; - m_input_stream.discard_or_error(length); + TRY(m_input_stream->read(subfield_id.bytes())); + TRY(m_input_stream->read(length.bytes())); + TRY(m_input_stream->discard(length)); } - auto discard_string = [&]() { + auto discard_string = [&]() -> ErrorOr { char next_char; do { - m_input_stream >> next_char; - if (m_input_stream.has_any_error()) { - set_fatal_error(); - break; - } + TRY(m_input_stream->read({ &next_char, sizeof(next_char) })); } while (next_char); + + return {}; }; - if (header.flags & Flags::FNAME) { - discard_string(); - if (has_any_error()) - break; - } + if (header.flags & Flags::FNAME) + TRY(discard_string()); - if (header.flags & Flags::FCOMMENT) { - discard_string(); - if (has_any_error()) - break; - } + if (header.flags & Flags::FCOMMENT) + TRY(discard_string()); if (header.flags & Flags::FHCRC) { LittleEndian crc16; - m_input_stream >> crc16; + TRY(m_input_stream->read(crc16.bytes())); // FIXME: we should probably verify this instead of just assuming it matches } - m_current_member.emplace(header, m_input_stream); + m_current_member.emplace(header, *m_input_stream); continue; } } - return total_read; + return bytes.slice(0, total_read); } Optional GzipDecompressor::describe_header(ReadonlyBytes bytes) @@ -167,57 +154,26 @@ Optional GzipDecompressor::describe_header(ReadonlyBytes bytes return DeprecatedString::formatted("last modified: {}, original size {}", Core::DateTime::from_timestamp(header.modification_time).to_deprecated_string(), (u32)original_size); } -bool GzipDecompressor::read_or_error(Bytes bytes) +ErrorOr GzipDecompressor::decompress_all(ReadonlyBytes bytes) { - if (read(bytes) < bytes.size()) { - set_fatal_error(); - return false; - } - - return true; -} - -bool GzipDecompressor::discard_or_error(size_t count) -{ - u8 buffer[4096]; - - size_t ndiscarded = 0; - while (ndiscarded < count) { - if (unreliable_eof()) { - set_fatal_error(); - return false; - } - - ndiscarded += read({ buffer, min(count - ndiscarded, sizeof(buffer)) }); - } - - return true; -} - -Optional GzipDecompressor::decompress_all(ReadonlyBytes bytes) -{ - InputMemoryStream memory_stream { bytes }; - GzipDecompressor gzip_stream { memory_stream }; + auto memory_stream = TRY(Core::Stream::MemoryStream::construct(bytes)); + auto gzip_stream = make(move(memory_stream)); DuplexMemoryStream output_stream; - u8 buffer[4096]; - while (!gzip_stream.has_any_error() && !gzip_stream.unreliable_eof()) { - auto const nread = gzip_stream.read({ buffer, sizeof(buffer) }); - output_stream.write_or_error({ buffer, nread }); + auto buffer = TRY(ByteBuffer::create_uninitialized(4096)); + while (!gzip_stream->is_eof()) { + auto const data = TRY(gzip_stream->read(buffer)); + output_stream.write_or_error(data); } - if (gzip_stream.handle_any_error()) - return {}; - return output_stream.copy_into_contiguous_buffer(); } -bool GzipDecompressor::unreliable_eof() const { return m_eof; } +bool GzipDecompressor::is_eof() const { return m_input_stream->is_eof(); } -bool GzipDecompressor::handle_any_error() +ErrorOr GzipDecompressor::write(ReadonlyBytes) { - bool handled_errors = m_input_stream.handle_any_error(); - return Stream::handle_any_error() || handled_errors; + VERIFY_NOT_REACHED(); } GzipCompressor::GzipCompressor(OutputStream& stream) diff --git a/Userland/Libraries/LibCompress/Gzip.h b/Userland/Libraries/LibCompress/Gzip.h index 080683f52f..3616c5f659 100644 --- a/Userland/Libraries/LibCompress/Gzip.h +++ b/Userland/Libraries/LibCompress/Gzip.h @@ -8,6 +8,7 @@ #pragma once #include +#include #include namespace Compress { @@ -37,32 +38,33 @@ struct Flags { static constexpr u8 MAX = FTEXT | FHCRC | FEXTRA | FNAME | FCOMMENT; }; -class GzipDecompressor final : public InputStream { +class GzipDecompressor final : public Core::Stream::Stream { public: - GzipDecompressor(InputStream&); + GzipDecompressor(NonnullOwnPtr); ~GzipDecompressor(); - size_t read(Bytes) override; - bool read_or_error(Bytes) override; - bool discard_or_error(size_t) override; + virtual ErrorOr read(Bytes) override; + virtual ErrorOr write(ReadonlyBytes) override; + virtual bool is_eof() const override; + virtual bool is_open() const override { return true; } + virtual void close() override {}; - bool unreliable_eof() const override; - bool handle_any_error() override; - - static Optional decompress_all(ReadonlyBytes); + static ErrorOr decompress_all(ReadonlyBytes); static Optional describe_header(ReadonlyBytes); static bool is_likely_compressed(ReadonlyBytes bytes); private: class Member { public: - Member(BlockHeader header, InputStream& stream) + Member(BlockHeader header, Core::Stream::Stream& stream) : m_header(header) - , m_stream(stream) + , m_adapted_ak_stream(make(stream)) + , m_stream(*m_adapted_ak_stream) { } BlockHeader m_header; + NonnullOwnPtr m_adapted_ak_stream; DeflateDecompressor m_stream; Crypto::Checksum::CRC32 m_checksum; size_t m_nread { 0 }; @@ -71,7 +73,7 @@ private: Member const& current_member() const { return m_current_member.value(); } Member& current_member() { return m_current_member.value(); } - InputStream& m_input_stream; + NonnullOwnPtr m_input_stream; u8 m_partial_header[sizeof(BlockHeader)]; size_t m_partial_header_offset { 0 }; Optional m_current_member; diff --git a/Userland/Libraries/LibCoredump/Reader.cpp b/Userland/Libraries/LibCoredump/Reader.cpp index c5f80cc5ef..006bb8d334 100644 --- a/Userland/Libraries/LibCoredump/Reader.cpp +++ b/Userland/Libraries/LibCoredump/Reader.cpp @@ -67,8 +67,8 @@ Reader::Reader(ReadonlyBytes coredump_bytes) Optional Reader::decompress_coredump(ReadonlyBytes raw_coredump) { auto decompressed_coredump = Compress::GzipDecompressor::decompress_all(raw_coredump); - if (decompressed_coredump.has_value()) - return decompressed_coredump; + if (!decompressed_coredump.is_error()) + return decompressed_coredump.release_value(); // If we didn't manage to decompress it, try and parse it as decompressed coredump auto bytebuffer = ByteBuffer::copy(raw_coredump); diff --git a/Userland/Libraries/LibHTTP/Job.cpp b/Userland/Libraries/LibHTTP/Job.cpp index 25c04d951c..8e08879d83 100644 --- a/Userland/Libraries/LibHTTP/Job.cpp +++ b/Userland/Libraries/LibHTTP/Job.cpp @@ -37,8 +37,8 @@ static Optional handle_content_encoding(ByteBuffer const& buf, Depre dbgln_if(JOB_DEBUG, "Job::handle_content_encoding: buf is gzip compressed!"); auto uncompressed = Compress::GzipDecompressor::decompress_all(buf); - if (!uncompressed.has_value()) { - dbgln("Job::handle_content_encoding: Gzip::decompress() failed."); + if (uncompressed.is_error()) { + dbgln("Job::handle_content_encoding: Gzip::decompress() failed: {}", uncompressed.error()); return {}; } diff --git a/Userland/Utilities/gunzip.cpp b/Userland/Utilities/gunzip.cpp index a27c310cc6..6948f3ae6a 100644 --- a/Userland/Utilities/gunzip.cpp +++ b/Userland/Utilities/gunzip.cpp @@ -11,18 +11,17 @@ #include #include -static bool decompress_file(Buffered& input_stream, Buffered& output_stream) +static ErrorOr decompress_file(NonnullOwnPtr input_stream, Buffered& output_stream) { - auto gzip_stream = Compress::GzipDecompressor { input_stream }; + auto gzip_stream = Compress::GzipDecompressor { move(input_stream) }; - u8 buffer[4096]; - - while (!gzip_stream.has_any_error() && !gzip_stream.unreliable_eof()) { - auto const nread = gzip_stream.read({ buffer, sizeof(buffer) }); - output_stream.write_or_error({ buffer, nread }); + auto buffer = TRY(ByteBuffer::create_uninitialized(4096)); + while (!gzip_stream.is_eof()) { + auto span = TRY(gzip_stream.read(buffer)); + output_stream.write_or_error(span); } - return !gzip_stream.handle_any_error(); + return {}; } ErrorOr serenity_main(Main::Arguments args) @@ -52,20 +51,15 @@ ErrorOr serenity_main(Main::Arguments args) output_filename = filename; } - auto input_stream_result = TRY(Core::InputFileStream::open_buffered(input_filename)); + auto input_stream_result = TRY(Core::Stream::File::open(input_filename, Core::Stream::OpenMode::Read)); - auto success = false; if (write_to_stdout) { auto stdout = Core::OutputFileStream::stdout_buffered(); - success = decompress_file(input_stream_result, stdout); + TRY(decompress_file(move(input_stream_result), stdout)); } else { auto output_stream_result = TRY(Core::OutputFileStream::open_buffered(output_filename)); - success = decompress_file(input_stream_result, output_stream_result); - } - if (!success) { - warnln("Failed gzip decompressing input file"); - return 1; + TRY(decompress_file(move(input_stream_result), output_stream_result)); } if (!keep_input_files) diff --git a/Userland/Utilities/gzip.cpp b/Userland/Utilities/gzip.cpp index 114b53fd7a..991ff21674 100644 --- a/Userland/Utilities/gzip.cpp +++ b/Userland/Utilities/gzip.cpp @@ -52,7 +52,7 @@ ErrorOr serenity_main(Main::Arguments arguments) AK::Optional output_bytes; if (decompress) - output_bytes = Compress::GzipDecompressor::decompress_all(input_bytes); + output_bytes = TRY(Compress::GzipDecompressor::decompress_all(input_bytes)); else output_bytes = Compress::GzipCompressor::compress_all(input_bytes); diff --git a/Userland/Utilities/tar.cpp b/Userland/Utilities/tar.cpp index f726c9f666..6e0aebdf83 100644 --- a/Userland/Utilities/tar.cpp +++ b/Userland/Utilities/tar.cpp @@ -65,27 +65,10 @@ ErrorOr serenity_main(Main::Arguments arguments) if (!directory.is_empty()) TRY(Core::System::chdir(directory)); - // FIXME: Remove these once we have smart pointers everywhere in LibArchive and LibCompress (or just ported the whole stack to Core::Stream). - // Until then, we have to hold on to _some_ instance of the file AK::Stream. - // Note that this is only in use together with gzip. - OwnPtr file_stream; + NonnullOwnPtr input_stream = TRY(Core::Stream::File::open_file_or_standard_stream(archive_file, Core::Stream::OpenMode::Read)); - auto input_stream = TRY([&]() -> ErrorOr> { - if (gzip) { - // FIXME: Port gzip to Core::Stream. - auto file = Core::File::standard_input(); - - if (!archive_file.is_empty()) - file = TRY(Core::File::open(archive_file, Core::OpenMode::ReadOnly)); - - file_stream = adopt_own(*new Core::InputFileStream(file)); - NonnullOwnPtr gzip_stream = make(*file_stream); - - return make(move(gzip_stream)); - } else { - return TRY(Core::Stream::File::open_file_or_standard_stream(archive_file, Core::Stream::OpenMode::Read)); - } - }()); + if (gzip) + input_stream = make(move(input_stream)); auto tar_stream = TRY(Archive::TarInputStream::construct(move(input_stream)));