From 28c99e7a1fd5f9b8c18f28c06e26a06cfe4d464f Mon Sep 17 00:00:00 2001 From: implicitfield <114500360+implicitfield@users.noreply.github.com> Date: Thu, 22 Dec 2022 16:21:13 +0200 Subject: [PATCH] LibArchive+Utilities: Stop using DeprecatedString This also slightly improves error propagation in tar, unzip and zip. --- Meta/Lagom/Fuzzers/FuzzZip.cpp | 2 +- Userland/Libraries/LibArchive/Tar.cpp | 6 +++-- Userland/Libraries/LibArchive/Tar.h | 23 +++++++++--------- Userland/Libraries/LibArchive/TarStream.cpp | 26 ++++++++++----------- Userland/Libraries/LibArchive/TarStream.h | 6 ++--- Userland/Libraries/LibArchive/Zip.cpp | 21 +++++++---------- Userland/Libraries/LibArchive/Zip.h | 6 ++--- Userland/Utilities/tar.cpp | 6 ++--- Userland/Utilities/unzip.cpp | 14 +++++------ Userland/Utilities/zip.cpp | 4 ++-- 10 files changed, 57 insertions(+), 57 deletions(-) diff --git a/Meta/Lagom/Fuzzers/FuzzZip.cpp b/Meta/Lagom/Fuzzers/FuzzZip.cpp index 9d2d669c0a4..4d84a59f866 100644 --- a/Meta/Lagom/Fuzzers/FuzzZip.cpp +++ b/Meta/Lagom/Fuzzers/FuzzZip.cpp @@ -14,7 +14,7 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) if (!zip_file.has_value()) return 0; - zip_file->for_each_member([](auto&) { + (void)zip_file->for_each_member([](auto&) { return IterationDecision::Continue; }); diff --git a/Userland/Libraries/LibArchive/Tar.cpp b/Userland/Libraries/LibArchive/Tar.cpp index ebb968da1ee..457ab0c07d0 100644 --- a/Userland/Libraries/LibArchive/Tar.cpp +++ b/Userland/Libraries/LibArchive/Tar.cpp @@ -24,10 +24,12 @@ unsigned TarFileHeader::expected_checksum() const return checksum; } -void TarFileHeader::calculate_checksum() +ErrorOr TarFileHeader::calculate_checksum() { memset(m_checksum, ' ', sizeof(m_checksum)); - VERIFY(DeprecatedString::formatted("{:06o}", expected_checksum()).copy_characters_to_buffer(m_checksum, sizeof(m_checksum))); + bool copy_successful = TRY(String::formatted("{:06o}", expected_checksum())).bytes_as_string_view().copy_characters_to_buffer(m_checksum, sizeof(m_checksum)); + VERIFY(copy_successful); + return {}; } bool TarFileHeader::is_zero_block() const diff --git a/Userland/Libraries/LibArchive/Tar.h b/Userland/Libraries/LibArchive/Tar.h index c5741dc6de1..8cb1c18c143 100644 --- a/Userland/Libraries/LibArchive/Tar.h +++ b/Userland/Libraries/LibArchive/Tar.h @@ -8,7 +8,7 @@ #pragma once -#include +#include #include #include #include @@ -82,9 +82,10 @@ static void set_field(char (&field)[N], TSource&& source) } template -static void set_octal_field(char (&field)[N], TSource&& source) +static ErrorOr set_octal_field(char (&field)[N], TSource&& source) { - set_field(field, DeprecatedString::formatted("{:o}", forward(source))); + set_field(field, TRY(String::formatted("{:o}", forward(source))).bytes_as_string_view()); + return {}; } class [[gnu::packed]] TarFileHeader { @@ -109,11 +110,11 @@ public: StringView prefix() const { return get_field_as_string_view(m_prefix); } void set_filename(StringView filename) { set_field(m_filename, filename); } - void set_mode(mode_t mode) { set_octal_field(m_mode, mode); } - void set_uid(uid_t uid) { set_octal_field(m_uid, uid); } - void set_gid(gid_t gid) { set_octal_field(m_gid, gid); } - void set_size(size_t size) { set_octal_field(m_size, size); } - void set_timestamp(time_t timestamp) { set_octal_field(m_timestamp, timestamp); } + ErrorOr set_mode(mode_t mode) { return set_octal_field(m_mode, mode); } + ErrorOr set_uid(uid_t uid) { return set_octal_field(m_uid, uid); } + ErrorOr set_gid(gid_t gid) { return set_octal_field(m_gid, gid); } + ErrorOr set_size(size_t size) { return set_octal_field(m_size, size); } + ErrorOr set_timestamp(time_t timestamp) { return set_octal_field(m_timestamp, timestamp); } void set_type_flag(TarFileType type) { m_type_flag = to_underlying(type); } void set_link_name(StringView link_name) { set_field(m_link_name, link_name); } // magic doesn't necessarily include a null byte @@ -122,12 +123,12 @@ public: void set_version(StringView version) { set_field(m_version, version); } void set_owner_name(StringView owner_name) { set_field(m_owner_name, owner_name); } void set_group_name(StringView group_name) { set_field(m_group_name, group_name); } - void set_major(int major) { set_octal_field(m_major, major); } - void set_minor(int minor) { set_octal_field(m_minor, minor); } + ErrorOr set_major(int major) { return set_octal_field(m_major, major); } + ErrorOr set_minor(int minor) { return set_octal_field(m_minor, minor); } void set_prefix(StringView prefix) { set_field(m_prefix, prefix); } unsigned expected_checksum() const; - void calculate_checksum(); + ErrorOr calculate_checksum(); bool is_zero_block() const; bool content_is_like_extended_header() const; diff --git a/Userland/Libraries/LibArchive/TarStream.cpp b/Userland/Libraries/LibArchive/TarStream.cpp index fcac3c8a255..7ca219f3d80 100644 --- a/Userland/Libraries/LibArchive/TarStream.cpp +++ b/Userland/Libraries/LibArchive/TarStream.cpp @@ -142,34 +142,34 @@ TarOutputStream::TarOutputStream(Core::Stream::Handle stre { } -ErrorOr TarOutputStream::add_directory(DeprecatedString const& path, mode_t mode) +ErrorOr TarOutputStream::add_directory(StringView path, mode_t mode) { VERIFY(!m_finished); TarFileHeader header {}; - header.set_size(0); - header.set_filename_and_prefix(DeprecatedString::formatted("{}/", path)); // Old tar implementations assume directory names end with a / + TRY(header.set_size(0)); + header.set_filename_and_prefix(TRY(String::formatted("{}/", path))); // Old tar implementations assume directory names end with a / header.set_type_flag(TarFileType::Directory); - header.set_mode(mode); + TRY(header.set_mode(mode)); header.set_magic(gnu_magic); header.set_version(gnu_version); - header.calculate_checksum(); + TRY(header.calculate_checksum()); TRY(m_stream->write_entire_buffer(Bytes { &header, sizeof(header) })); u8 padding[block_size] = { 0 }; TRY(m_stream->write_entire_buffer(Bytes { &padding, block_size - sizeof(header) })); return {}; } -ErrorOr TarOutputStream::add_file(DeprecatedString const& path, mode_t mode, ReadonlyBytes bytes) +ErrorOr TarOutputStream::add_file(StringView path, mode_t mode, ReadonlyBytes bytes) { VERIFY(!m_finished); TarFileHeader header {}; - header.set_size(bytes.size()); + TRY(header.set_size(bytes.size())); header.set_filename_and_prefix(path); header.set_type_flag(TarFileType::NormalFile); - header.set_mode(mode); + TRY(header.set_mode(mode)); header.set_magic(gnu_magic); header.set_version(gnu_version); - header.calculate_checksum(); + TRY(header.calculate_checksum()); TRY(m_stream->write_entire_buffer(ReadonlyBytes { &header, sizeof(header) })); constexpr Array padding { 0 }; TRY(m_stream->write_entire_buffer(ReadonlyBytes { &padding, block_size - sizeof(header) })); @@ -181,18 +181,18 @@ ErrorOr TarOutputStream::add_file(DeprecatedString const& path, mode_t mod return {}; } -ErrorOr TarOutputStream::add_link(DeprecatedString const& path, mode_t mode, StringView link_name) +ErrorOr TarOutputStream::add_link(StringView path, mode_t mode, StringView link_name) { VERIFY(!m_finished); TarFileHeader header {}; - header.set_size(0); + TRY(header.set_size(0)); header.set_filename_and_prefix(path); header.set_type_flag(TarFileType::SymLink); - header.set_mode(mode); + TRY(header.set_mode(mode)); header.set_magic(gnu_magic); header.set_version(gnu_version); header.set_link_name(link_name); - header.calculate_checksum(); + TRY(header.calculate_checksum()); TRY(m_stream->write_entire_buffer(Bytes { &header, sizeof(header) })); u8 padding[block_size] = { 0 }; TRY(m_stream->write_entire_buffer(Bytes { &padding, block_size - sizeof(header) })); diff --git a/Userland/Libraries/LibArchive/TarStream.h b/Userland/Libraries/LibArchive/TarStream.h index cd571e94da3..0efb41da2e2 100644 --- a/Userland/Libraries/LibArchive/TarStream.h +++ b/Userland/Libraries/LibArchive/TarStream.h @@ -59,9 +59,9 @@ private: class TarOutputStream { public: TarOutputStream(Core::Stream::Handle); - ErrorOr add_file(DeprecatedString const& path, mode_t, ReadonlyBytes); - ErrorOr add_link(DeprecatedString const& path, mode_t, StringView); - ErrorOr add_directory(DeprecatedString const& path, mode_t); + ErrorOr add_file(StringView path, mode_t, ReadonlyBytes); + ErrorOr add_link(StringView path, mode_t, StringView); + ErrorOr add_directory(StringView path, mode_t); ErrorOr finish(); private: diff --git a/Userland/Libraries/LibArchive/Zip.cpp b/Userland/Libraries/LibArchive/Zip.cpp index 45af7cd9efd..366f50338c7 100644 --- a/Userland/Libraries/LibArchive/Zip.cpp +++ b/Userland/Libraries/LibArchive/Zip.cpp @@ -75,7 +75,7 @@ Optional Zip::try_create(ReadonlyBytes buffer) }; } -bool Zip::for_each_member(Function callback) +ErrorOr Zip::for_each_member(Function callback) { size_t member_offset = m_members_start_offset; for (size_t i = 0; i < m_member_count; i++) { @@ -85,15 +85,12 @@ bool Zip::for_each_member(Function callback VERIFY(local_file_header.read(m_input_data.slice(central_directory_record.local_file_header_offset))); ZipMember member; - char null_terminated_name[central_directory_record.name_length + 1]; - memcpy(null_terminated_name, central_directory_record.name, central_directory_record.name_length); - null_terminated_name[central_directory_record.name_length] = 0; - member.name = DeprecatedString { null_terminated_name }; + member.name = TRY(String::from_utf8({ central_directory_record.name, central_directory_record.name_length })); member.compressed_data = { local_file_header.compressed_data, central_directory_record.compressed_size }; member.compression_method = central_directory_record.compression_method; member.uncompressed_size = central_directory_record.uncompressed_size; member.crc32 = central_directory_record.crc32; - member.is_directory = central_directory_record.external_attributes & zip_directory_external_attribute || member.name.ends_with('/'); // FIXME: better directory detection + member.is_directory = central_directory_record.external_attributes & zip_directory_external_attribute || member.name.bytes_as_string_view().ends_with('/'); // FIXME: better directory detection if (callback(member) == IterationDecision::Break) return false; @@ -117,7 +114,7 @@ static u16 minimum_version_needed(ZipCompressionMethod method) ErrorOr ZipOutputStream::add_member(ZipMember const& member) { VERIFY(!m_finished); - VERIFY(member.name.length() <= UINT16_MAX); + VERIFY(member.name.bytes_as_string_view().length() <= UINT16_MAX); VERIFY(member.compressed_data.size() <= UINT32_MAX); TRY(m_members.try_append(member)); @@ -130,9 +127,9 @@ ErrorOr ZipOutputStream::add_member(ZipMember const& member) .crc32 = member.crc32, .compressed_size = static_cast(member.compressed_data.size()), .uncompressed_size = member.uncompressed_size, - .name_length = static_cast(member.name.length()), + .name_length = static_cast(member.name.bytes_as_string_view().length()), .extra_data_length = 0, - .name = reinterpret_cast(member.name.characters()), + .name = reinterpret_cast(member.name.bytes_as_string_view().characters_without_null_termination()), .extra_data = nullptr, .compressed_data = member.compressed_data.data(), }; @@ -158,18 +155,18 @@ ErrorOr ZipOutputStream::finish() .crc32 = member.crc32, .compressed_size = static_cast(member.compressed_data.size()), .uncompressed_size = member.uncompressed_size, - .name_length = static_cast(member.name.length()), + .name_length = static_cast(member.name.bytes_as_string_view().length()), .extra_data_length = 0, .comment_length = 0, .start_disk = 0, .internal_attributes = 0, .external_attributes = member.is_directory ? zip_directory_external_attribute : 0, .local_file_header_offset = file_header_offset, // FIXME: we assume the wrapped output stream was never written to before us - .name = reinterpret_cast(member.name.characters()), + .name = reinterpret_cast(member.name.bytes_as_string_view().characters_without_null_termination()), .extra_data = nullptr, .comment = nullptr, }; - file_header_offset += sizeof(LocalFileHeader::signature) + (sizeof(LocalFileHeader) - (sizeof(u8*) * 3)) + member.name.length() + member.compressed_data.size(); + file_header_offset += sizeof(LocalFileHeader::signature) + (sizeof(LocalFileHeader) - (sizeof(u8*) * 3)) + member.name.bytes_as_string_view().length() + member.compressed_data.size(); TRY(central_directory_record.write(*m_stream)); central_directory_size += central_directory_record.size(); } diff --git a/Userland/Libraries/LibArchive/Zip.h b/Userland/Libraries/LibArchive/Zip.h index 4ce3572c53c..a4cec32048a 100644 --- a/Userland/Libraries/LibArchive/Zip.h +++ b/Userland/Libraries/LibArchive/Zip.h @@ -8,9 +8,9 @@ #pragma once #include -#include #include #include +#include #include #include #include @@ -238,7 +238,7 @@ struct [[gnu::packed]] LocalFileHeader { }; struct ZipMember { - DeprecatedString name; + String name; ReadonlyBytes compressed_data; // TODO: maybe the decompression/compression should be handled by LibArchive instead of the user? ZipCompressionMethod compression_method; u32 uncompressed_size; @@ -249,7 +249,7 @@ struct ZipMember { class Zip { public: static Optional try_create(ReadonlyBytes buffer); - bool for_each_member(Function); + ErrorOr for_each_member(Function); private: static bool find_end_of_central_directory_offset(ReadonlyBytes, size_t& offset); diff --git a/Userland/Utilities/tar.cpp b/Userland/Utilities/tar.cpp index 73473d52302..55355d07e9d 100644 --- a/Userland/Utilities/tar.cpp +++ b/Userland/Utilities/tar.cpp @@ -228,7 +228,7 @@ ErrorOr serenity_main(Main::Arguments arguments) } auto statbuf = TRY(Core::System::lstat(path)); - auto canonicalized_path = LexicalPath::canonicalized_path(path); + auto canonicalized_path = TRY(String::from_deprecated_string(LexicalPath::canonicalized_path(path))); TRY(tar_stream.add_file(canonicalized_path, statbuf.st_mode, file->read_all())); if (verbose) outln("{}", canonicalized_path); @@ -239,7 +239,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto add_link = [&](DeprecatedString path) -> ErrorOr { auto statbuf = TRY(Core::System::lstat(path)); - auto canonicalized_path = LexicalPath::canonicalized_path(path); + auto canonicalized_path = TRY(String::from_deprecated_string(LexicalPath::canonicalized_path(path))); TRY(tar_stream.add_link(canonicalized_path, statbuf.st_mode, TRY(Core::System::readlink(path)))); if (verbose) outln("{}", canonicalized_path); @@ -250,7 +250,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto add_directory = [&](DeprecatedString path, auto handle_directory) -> ErrorOr { auto statbuf = TRY(Core::System::lstat(path)); - auto canonicalized_path = LexicalPath::canonicalized_path(path); + auto canonicalized_path = TRY(String::from_deprecated_string(LexicalPath::canonicalized_path(path))); TRY(tar_stream.add_directory(canonicalized_path, statbuf.st_mode)); if (verbose) outln("{}", canonicalized_path); diff --git a/Userland/Utilities/unzip.cpp b/Userland/Utilities/unzip.cpp index c940b2453e1..293acf903c2 100644 --- a/Userland/Utilities/unzip.cpp +++ b/Userland/Utilities/unzip.cpp @@ -20,16 +20,16 @@ static bool unpack_zip_member(Archive::ZipMember zip_member, bool quiet) { if (zip_member.is_directory) { - if (mkdir(zip_member.name.characters(), 0755) < 0) { - perror("mkdir"); + if (auto maybe_error = Core::System::mkdir(zip_member.name, 0755); maybe_error.is_error()) { + warnln("Failed to create directory '{}': {}", zip_member.name, maybe_error.error()); return false; } if (!quiet) outln(" extracting: {}", zip_member.name); return true; } - MUST(Core::Directory::create(LexicalPath(zip_member.name).parent(), Core::Directory::CreateDirectories::Yes)); - auto new_file = Core::File::construct(zip_member.name); + MUST(Core::Directory::create(LexicalPath(zip_member.name.to_deprecated_string()).parent(), Core::Directory::CreateDirectories::Yes)); + auto new_file = Core::File::construct(zip_member.name.to_deprecated_string()); if (!new_file->open(Core::OpenMode::WriteOnly)) { warnln("Can't write file {}: {}", zip_member.name, new_file->error_string()); return false; @@ -123,14 +123,14 @@ ErrorOr serenity_main(Main::Arguments arguments) TRY(Core::System::chdir(output_directory_path)); } - auto success = zip_file->for_each_member([&](auto zip_member) { + auto success = TRY(zip_file->for_each_member([&](auto zip_member) { bool keep_file = false; if (!file_filters.is_empty()) { for (auto& filter : file_filters) { // Convert underscore wildcards (usual unzip convention) to question marks (as used by StringUtils) auto string_filter = filter.replace("_"sv, "?"sv, ReplaceMode::All); - if (zip_member.name.matches(string_filter, CaseSensitivity::CaseSensitive)) { + if (zip_member.name.bytes_as_string_view().matches(string_filter, CaseSensitivity::CaseSensitive)) { keep_file = true; break; } @@ -145,7 +145,7 @@ ErrorOr serenity_main(Main::Arguments arguments) } return IterationDecision::Continue; - }); + })); return success ? 0 : 1; } diff --git a/Userland/Utilities/zip.cpp b/Userland/Utilities/zip.cpp index b04dc9bd32c..2fcb15b9c54 100644 --- a/Userland/Utilities/zip.cpp +++ b/Userland/Utilities/zip.cpp @@ -56,7 +56,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto file = TRY(Core::Stream::File::open(path, Core::Stream::OpenMode::Read)); auto file_buffer = TRY(file->read_until_eof()); Archive::ZipMember member {}; - member.name = canonicalized_path; + member.name = TRY(String::from_deprecated_string(canonicalized_path)); auto deflate_buffer = Compress::DeflateCompressor::compress_all(file_buffer); if (deflate_buffer.has_value() && deflate_buffer.value().size() < file_buffer.size()) { @@ -79,7 +79,7 @@ ErrorOr serenity_main(Main::Arguments arguments) auto add_directory = [&](DeprecatedString path, auto handle_directory) -> ErrorOr { auto canonicalized_path = DeprecatedString::formatted("{}/", LexicalPath::canonicalized_path(path)); Archive::ZipMember member {}; - member.name = canonicalized_path; + member.name = TRY(String::from_deprecated_string(canonicalized_path)); member.compressed_data = {}; member.compression_method = Archive::ZipCompressionMethod::Store; member.uncompressed_size = 0;