diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a2d04096..f7624e1e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Stream copies report failed reads and writes, and file writes check their + final flush. Saving a text file over its source reads the text before truncation. + - Shared text helpers keep ASCII matching independent of locale and reject malformed UTF-8, empty replacement patterns and overflowing repetitions. In-memory byte writes reject offsets that overflow their bounds checks. diff --git a/src/odr/file.cpp b/src/odr/file.cpp index e6d4d3008..8ce90e278 100644 --- a/src/odr/file.cpp +++ b/src/odr/file.cpp @@ -346,8 +346,7 @@ void TextFile::save(const std::string &path) const { if (!is_savable()) { throw UnsupportedOperation(); } - std::ofstream out = internal::util::file::create(path); - out << text(); + internal::util::file::write(text(), path); } void TextFile::save(std::ostream &out) const { diff --git a/src/odr/internal/common/temporary_file.cpp b/src/odr/internal/common/temporary_file.cpp index 9dc2a92f8..68c73c499 100644 --- a/src/odr/internal/common/temporary_file.cpp +++ b/src/odr/internal/common/temporary_file.cpp @@ -1,5 +1,7 @@ #include +#include + #include #include @@ -87,14 +89,26 @@ TemporaryDiskFile TemporaryDiskFileFactory::copy(std::istream &in) const { if (!file.is_open()) { file.clear(); file.open(file_path.string(), std::ios_base::out | std::ios_base::binary); + if (!file.is_open()) { + throw FileWriteError(file_path.string()); + } break; } file.close(); } - util::stream::pipe(in, file); - file.close(); + try { + util::stream::pipe(in, file); + file.close(); + if (!file) { + throw FileWriteError(file_path.string()); + } + } catch (...) { + file.close(); + remove_quietly(file_path); + throw; + } return TemporaryDiskFile(file_path); } diff --git a/src/odr/internal/encoding/detect.cpp b/src/odr/internal/encoding/detect.cpp index f8d9dbd42..8dbc9f713 100644 --- a/src/odr/internal/encoding/detect.cpp +++ b/src/odr/internal/encoding/detect.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include @@ -49,10 +50,7 @@ bool encoding_holds_nul(const TextEncoding encoding) { std::string encoding::read_probe(std::istream &in, const std::size_t max_bytes) { - std::string result(max_bytes, '\0'); - in.read(result.data(), static_cast(max_bytes)); - result.resize(static_cast(in.gcount())); - return result; + return util::stream::read(in, max_bytes); } TextEncoding encoding::detect(const std::string_view probe) { diff --git a/src/odr/internal/magic.cpp b/src/odr/internal/magic.cpp index fcf13e685..81239bc2a 100644 --- a/src/odr/internal/magic.cpp +++ b/src/odr/internal/magic.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -18,14 +19,6 @@ namespace odr::internal { namespace { -/// At most @p size bytes, cut back to what was actually read. -std::string read_head(std::istream &in, const std::size_t size) { - std::string result(size, '\0'); - in.read(result.data(), static_cast(size)); - result.resize(static_cast(in.gcount())); - return result; -} - bool match_magic(const std::string &head, const std::string &pattern) { const auto bytes = util::string::split(pattern, " "); if (bytes.size() > head.size()) { @@ -201,7 +194,7 @@ FileType magic::file_type(std::istream &in) { // an enhanced metafile names itself at offset 40, so the head has to reach 44 static constexpr std::size_t head_size = 64; - return file_type(read_head(in, head_size)); + return file_type(util::stream::read(in, head_size)); } FileType magic::file_type(const abstract::File &file) { diff --git a/src/odr/internal/util/file_util.cpp b/src/odr/internal/util/file_util.cpp index 51a3bae15..8dec5e6f5 100644 --- a/src/odr/internal/util/file_util.cpp +++ b/src/odr/internal/util/file_util.cpp @@ -21,21 +21,17 @@ std::ifstream file::open(const std::string &path) { std::size_t file::size(const std::string &path) { std::ifstream in = open(path); in.seekg(0, std::ios::end); - return static_cast(in.tellg()); + const auto end = in.tellg(); + if (end < 0) { + throw FileReadError(); + } + return static_cast(end); } std::string file::read(const std::string &path) { std::ifstream in = open(path); - std::string result; - - in.seekg(0, std::ios::end); - result.reserve(in.tellg()); - in.seekg(0, std::ios::beg); - - result.assign(std::istreambuf_iterator(in), std::istreambuf_iterator()); - - return result; + return stream::read(in); } void file::pipe(const std::string &path, std::ostream &out) { @@ -54,11 +50,19 @@ std::ofstream file::create(const std::string &path) { void file::write(const std::string &data, const std::string &path) { std::ofstream out = create(path); out << data; + out.close(); + if (!out) { + throw FileWriteError(path); + } } void file::write(std::istream &in, const std::string &path) { std::ofstream out = create(path); stream::pipe(in, out); + out.close(); + if (!out) { + throw FileWriteError(path); + } } } // namespace odr::internal::util diff --git a/src/odr/internal/util/stream_util.cpp b/src/odr/internal/util/stream_util.cpp index bdc107f1e..c9aa3c845 100644 --- a/src/odr/internal/util/stream_util.cpp +++ b/src/odr/internal/util/stream_util.cpp @@ -1,7 +1,7 @@ #include #include -#include +#include #include #include @@ -12,12 +12,21 @@ using int_type = std::streambuf::int_type; static constexpr int_type eof = std::streambuf::traits_type::eof(); std::string stream::read(std::istream &in) { - return {std::istreambuf_iterator(in), {}}; + std::ostringstream out; + pipe(in, out); + return std::move(out).str(); } std::string stream::read(std::istream &in, const std::size_t size) { + if (size > + static_cast(std::numeric_limits::max())) { + throw std::length_error("stream read is too large"); + } std::string result(size, '\0'); in.read(result.data(), static_cast(size)); + if (in.bad() || (in.fail() && !in.eof())) { + throw std::ios_base::failure("stream read failed"); + } result.resize(static_cast(in.gcount())); return result; } @@ -29,11 +38,17 @@ void stream::pipe(std::istream &in, std::ostream &out) { while (true) { in.read(buffer.data(), BUFFER_SIZE); + if (in.bad() || (in.fail() && !in.eof())) { + throw std::ios_base::failure("stream read failed"); + } const auto read = in.gcount(); if (read == 0) { break; } out.write(buffer.data(), read); + if (!out) { + throw std::ios_base::failure("stream write failed"); + } } } diff --git a/test/src/file_test.cpp b/test/src/file_test.cpp index 7ed6a1262..7a4d919a3 100644 --- a/test/src/file_test.cpp +++ b/test/src/file_test.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -155,6 +156,29 @@ TEST(File, temporary_copies_preserve_binary_bytes) { EXPECT_EQ(internal::MemoryFile(copy).content(), data); } +TEST(File, text_can_be_saved_over_its_disk_source) { + std::istringstream source("original text\n"); + const auto temporary = + internal::TemporaryDiskFileFactory::system_default().copy(source); + const std::string path = temporary.disk_path()->string(); + const TextFile text = + open(path, DecodeOptions::as(FileType::text_file)).as_text_file(); + text.save(path); + EXPECT_EQ(internal::util::file::read(path), "original text\n"); +} + +TEST(File, failed_temporary_copies_leave_no_file) { + const internal::AbsPath directory(std::filesystem::temp_directory_path()); + const std::string name = "odr-failed-copy-" + internal::random_string(12); + const internal::TemporaryDiskFileFactory factory(directory, + [name] { return name; }); + std::istringstream source("bytes"); + source.setstate(std::ios::badbit); + EXPECT_THROW(std::ignore = factory.copy(source), std::ios_base::failure); + EXPECT_FALSE( + std::filesystem::exists(directory.join(internal::RelPath(name)).path())); +} + /// A file inside an archive is named by its entry, not by the archive. TEST(File, an_archive_entry_is_named_by_its_entry) { internal::zip::ZipArchive zip; diff --git a/test/src/internal/util/stream_util_test.cpp b/test/src/internal/util/stream_util_test.cpp index 3a66830a4..699fbeab7 100644 --- a/test/src/internal/util/stream_util_test.cpp +++ b/test/src/internal/util/stream_util_test.cpp @@ -86,6 +86,28 @@ TEST(Stream, delimiter_reads_preserve_failed_input) { EXPECT_EQ(stream::read_until(in, ';', true), "second;"); } +TEST(Stream, copying_reports_input_and_output_failures) { + std::istringstream in(std::string(4097, 'x')); + std::ostringstream out; + stream::pipe(in, out); + EXPECT_EQ(out.str(), std::string(4097, 'x')); + + in.clear(); + in.seekg(0); + out.setstate(std::ios::badbit); + EXPECT_THROW(stream::pipe(in, out), std::ios_base::failure); + + in.setstate(std::ios::badbit); + EXPECT_THROW(stream::read(in), std::ios_base::failure); + EXPECT_THROW(stream::read(in, 1), std::ios_base::failure); + + class FailingBuffer final : public std::streambuf { + int_type underflow() override { throw std::runtime_error("read failure"); } + } buffer; + std::istream broken(&buffer); + EXPECT_THROW(stream::read(broken), std::ios_base::failure); +} + TEST(ByteStream, length_prefixed_reads_handle_chunks_and_stream_exceptions) { const std::string data(4097, 'x'); std::istringstream in(data);