From 979a7a1dccafe86eb253983252e293bb2493fd62 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 7 Sep 2020 20:46:53 +0200 Subject: [PATCH 1/4] test/run_input: add option --chunk-size --- test/run_input.cxx | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/test/run_input.cxx b/test/run_input.cxx index 63e5f2ab7..9af34e8d2 100644 --- a/test/run_input.cxx +++ b/test/run_input.cxx @@ -49,11 +49,15 @@ #include #include +static constexpr std::size_t MAX_CHUNK_SIZE = 16384; + struct CommandLine { const char *uri = nullptr; FromNarrowPath config_path; + std::size_t chunk_size = MAX_CHUNK_SIZE; + bool verbose = false; bool scan = false; @@ -63,14 +67,27 @@ enum Option { OPTION_CONFIG, OPTION_VERBOSE, OPTION_SCAN, + OPTION_CHUNK_SIZE, }; static constexpr OptionDef option_defs[] = { {"config", 0, true, "Load a MPD configuration file"}, {"verbose", 'v', false, "Verbose logging"}, {"scan", 0, false, "Scan tags instead of reading raw data"}, + {"chunk-size", 0, true, "Read this number of bytes at a time"}, }; +static std::size_t +ParseSize(const char *s) +{ + char *endptr; + std::size_t value = std::strtoul(s, &endptr, 10); + if (endptr == s) + throw std::runtime_error("Failed to parse integer"); + + return value; +} + static CommandLine ParseCommandLine(int argc, char **argv) { @@ -90,6 +107,12 @@ ParseCommandLine(int argc, char **argv) case OPTION_SCAN: c.scan = true; break; + + case OPTION_CHUNK_SIZE: + c.chunk_size = ParseSize(o.value); + if (c.chunk_size <= 0 || c.chunk_size > MAX_CHUNK_SIZE) + throw std::runtime_error("Invalid chunk size"); + break; } } @@ -130,7 +153,7 @@ tag_save(FILE *file, const Tag &tag) } static int -dump_input_stream(InputStream &is, FileDescriptor out) +dump_input_stream(InputStream &is, FileDescriptor out, size_t chunk_size) { const std::lock_guard protect(is.mutex); @@ -150,8 +173,9 @@ dump_input_stream(InputStream &is, FileDescriptor out) } } - char buffer[4096]; - size_t num_read = is.Read(buffer, sizeof(buffer)); + char buffer[MAX_CHUNK_SIZE]; + assert(chunk_size <= sizeof(buffer)); + size_t num_read = is.Read(buffer, chunk_size); if (num_read == 0) break; @@ -232,7 +256,8 @@ try { Mutex mutex; auto is = InputStream::OpenReady(c.uri, mutex); - return dump_input_stream(*is, FileDescriptor(STDOUT_FILENO)); + return dump_input_stream(*is, FileDescriptor(STDOUT_FILENO), + c.chunk_size); } catch (...) { PrintException(std::current_exception()); return EXIT_FAILURE; From 1a5e0ef7c97cc50be7db0a1892a91110e538d46b Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 7 Sep 2020 20:52:54 +0200 Subject: [PATCH 2/4] test/test_archive_iso9660.sh: use an odd chunk size to trigger bug This makes the unit test fail. D'oh! --- test/test_archive_iso9660.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/test_archive_iso9660.sh b/test/test_archive_iso9660.sh index 5658f7a98..fc999bd47 100755 --- a/test/test_archive_iso9660.sh +++ b/test/test_archive_iso9660.sh @@ -7,4 +7,7 @@ DST="$(pwd)/test/tmp/${SRC_BASE}.iso" mkdir -p test/tmp rm -f "$DST" mkisofs -quiet -l -o "$DST" "$SRC" -./test/run_input "$DST/${SRC_BASE}" |diff "$SRC" - + +# Using an odd chunk size to check whether the plugin can cope with +# partial sectors +./test/run_input --chunk-size=1337 "$DST/${SRC_BASE}" |diff "$SRC" - From 17dd21ac7fc0667121f72e6ddc0a2b90d33b28ad Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 7 Sep 2020 13:12:08 +0200 Subject: [PATCH 3/4] archive/iso9660: fix unaligned reads Oh the horror! This plugin cannot possibly ever have worked. It was broken from the start, when it was added in commit 37796699cf7 nearly twelve (!) years ago. The plugin would always read at sector boundaries, so it could only ever work at multiples of 2 kB. --- NEWS | 1 + src/archive/plugins/Iso9660ArchivePlugin.cxx | 109 +++++++++++++++---- 2 files changed, 88 insertions(+), 22 deletions(-) diff --git a/NEWS b/NEWS index bbfbb271f..b80e5b19c 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,7 @@ ver 0.21.26 (not yet released) * archive - bzip2: fix crash on corrupt bzip2 file - bzip2: flush output at end of input file + - iso9660: fix unaligned reads - zzip: fix crash on corrupt ZIP file * decoder - sndfile: fix lost samples at end of file diff --git a/src/archive/plugins/Iso9660ArchivePlugin.cxx b/src/archive/plugins/Iso9660ArchivePlugin.cxx index 6314fb025..f81f6fd98 100644 --- a/src/archive/plugins/Iso9660ArchivePlugin.cxx +++ b/src/archive/plugins/Iso9660ArchivePlugin.cxx @@ -29,9 +29,12 @@ #include "fs/Path.hxx" #include "util/RuntimeError.hxx" #include "util/StringCompare.hxx" +#include "util/WritableBuffer.hxx" #include +#include + #include #include @@ -144,6 +147,47 @@ class Iso9660InputStream final : public InputStream { const lsn_t lsn; + /** + * libiso9660 can only read whole sectors at a time, and this + * buffer is used to store one whole sector and allow Read() + * to handle partial sector reads. + */ + class BlockBuffer { + size_t position = 0, fill = 0; + + std::array data; + + public: + ConstBuffer Read() const noexcept { + assert(fill <= data.size()); + assert(position <= fill); + + return {&data[position], &data[fill]}; + } + + void Consume(size_t nbytes) noexcept { + assert(nbytes <= Read().size); + + position += nbytes; + } + + WritableBuffer Write() noexcept { + assert(Read().empty()); + + return {data.data(), data.size()}; + } + + void Append(size_t nbytes) noexcept { + assert(Read().empty()); + assert(nbytes <= data.size()); + + fill = nbytes; + position = 0; + } + }; + + BlockBuffer buffer; + public: Iso9660InputStream(const std::shared_ptr &_iso, const char *_uri, @@ -182,35 +226,56 @@ Iso9660ArchiveFile::OpenStream(const char *pathname, size_t Iso9660InputStream::Read(void *ptr, size_t read_size) { - const ScopeUnlock unlock(mutex); - - int readed = 0; - int no_blocks, cur_block; - size_t left_bytes = size - offset; - - if (left_bytes < read_size) { - no_blocks = CEILING(left_bytes, ISO_BLOCKSIZE); - } else { - no_blocks = read_size / ISO_BLOCKSIZE; - } - - if (no_blocks == 0) + const offset_type remaining = size - offset; + if (remaining == 0) return 0; - cur_block = offset / ISO_BLOCKSIZE; + if (offset_type(read_size) > remaining) + read_size = remaining; - readed = iso->SeekRead(ptr, lsn + cur_block, no_blocks); + auto r = buffer.Read(); - if (readed != no_blocks * ISO_BLOCKSIZE) - throw FormatRuntimeError("error reading ISO file at lsn %lu", - (unsigned long)cur_block); + if (r.empty()) { + /* the buffer is empty - read more data from the ISO file */ - if (left_bytes < read_size) { - readed = left_bytes; + assert(offset % ISO_BLOCKSIZE == 0); + + const ScopeUnlock unlock(mutex); + + const lsn_t read_lsn = lsn + offset / ISO_BLOCKSIZE; + + if (read_size >= ISO_BLOCKSIZE) { + /* big read - read right into the caller's buffer */ + + auto nbytes = iso->SeekRead(ptr, read_lsn, + read_size / ISO_BLOCKSIZE); + if (nbytes <= 0) + throw std::runtime_error("Failed to read ISO9660 file"); + + offset += nbytes; + return nbytes; + } + + /* fill the buffer */ + + auto w = buffer.Write(); + auto nbytes = iso->SeekRead(w.data, read_lsn, + w.size / ISO_BLOCKSIZE); + if (nbytes <= 0) + throw std::runtime_error("Failed to read ISO9660 file"); + + buffer.Append(nbytes); + + r = buffer.Read(); } - offset += readed; - return readed; + assert(!r.empty()); + + size_t nbytes = std::min(read_size, r.size); + memcpy(ptr, r.data, nbytes); + buffer.Consume(nbytes); + offset += nbytes; + return nbytes; } bool From b2ae5298a765f83dd142564adfdcaf5b2d34ff52 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 31 May 2019 18:40:24 +0200 Subject: [PATCH 4/4] archive/iso9660: implement seeking --- NEWS | 1 + src/archive/plugins/Iso9660ArchivePlugin.cxx | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/NEWS b/NEWS index b80e5b19c..207c28226 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,7 @@ ver 0.21.26 (not yet released) - bzip2: fix crash on corrupt bzip2 file - bzip2: flush output at end of input file - iso9660: fix unaligned reads + - iso9660: support seeking - zzip: fix crash on corrupt ZIP file * decoder - sndfile: fix lost samples at end of file diff --git a/src/archive/plugins/Iso9660ArchivePlugin.cxx b/src/archive/plugins/Iso9660ArchivePlugin.cxx index f81f6fd98..be319d2a7 100644 --- a/src/archive/plugins/Iso9660ArchivePlugin.cxx +++ b/src/archive/plugins/Iso9660ArchivePlugin.cxx @@ -198,12 +198,20 @@ public: lsn(_lsn) { size = _size; + seekable = true; SetReady(); } /* virtual methods from InputStream */ bool IsEOF() noexcept override; size_t Read(void *ptr, size_t size) override; + + void Seek(offset_type new_offset) override { + if (new_offset > size) + throw std::runtime_error("Invalid seek offset"); + + offset = new_offset; + } }; InputStreamPtr