diff --git a/NEWS b/NEWS index f2d1364c9..0a33ffe8c 100644 --- a/NEWS +++ b/NEWS @@ -13,8 +13,6 @@ ver 0.22 (not yet released) - ffmpeg: allow partial reads - io_uring: new plugin for local files on Linux (using liburing) - smbclient: close unused SMB/CIFS connections -* archive - - iso9660: support seeking * database - upnp: drop support for libupnp versions older than 1.8 * playlist @@ -48,6 +46,8 @@ 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 + - 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 78b17d5b1..1976390a7 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 @@ -150,6 +153,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(std::shared_ptr _iso, const char *_uri, @@ -170,6 +214,9 @@ public: void *ptr, size_t size) override; void Seek(std::unique_lock &, offset_type new_offset) override { + if (new_offset > size) + throw std::runtime_error("Invalid seek offset"); + offset = new_offset; } }; @@ -195,35 +242,56 @@ size_t Iso9660InputStream::Read(std::unique_lock &, 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 diff --git a/test/run_input.cxx b/test/run_input.cxx index 57300183a..f24840f71 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) { std::unique_lock lock(is.mutex); @@ -150,8 +173,9 @@ dump_input_stream(InputStream &is, FileDescriptor out) } } - char buffer[4096]; - size_t num_read = is.Read(lock, buffer, sizeof(buffer)); + char buffer[MAX_CHUNK_SIZE]; + assert(chunk_size <= sizeof(buffer)); + size_t num_read = is.Read(lock, buffer, chunk_size); if (num_read == 0) break; @@ -231,7 +255,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; 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" -