From 932687f93d98a6c3f19b1836add72cb12396e66d Mon Sep 17 00:00:00 2001 From: Martchus Date: Sun, 15 Aug 2021 23:33:50 +0200 Subject: [PATCH] Improve warnings when parsing Vorbis comments --- ogg/oggiterator.cpp | 4 ++-- ogg/oggiterator.h | 18 ++++++++++++++++++ ogg/oggpage.cpp | 10 +++++++--- ogg/oggpage.h | 11 ++++++++++- vorbis/vorbiscomment.cpp | 18 ++++++++++++++++++ vorbis/vorbiscommentfield.cpp | 7 ++++--- 6 files changed, 59 insertions(+), 9 deletions(-) diff --git a/ogg/oggiterator.cpp b/ogg/oggiterator.cpp index 0aa361e..c6c8ec3 100644 --- a/ogg/oggiterator.cpp +++ b/ogg/oggiterator.cpp @@ -135,7 +135,7 @@ void OggIterator::read(char *buffer, std::size_t count) { std::size_t bytesRead = 0; while (*this && count) { - const auto available = currentSegmentSize() - m_bytesRead; + const auto available = remainingBytesInCurrentSegment(); stream().seekg(static_cast(currentCharacterOffset())); if (count <= available) { stream().read(buffer + bytesRead, static_cast(count)); @@ -169,7 +169,7 @@ std::size_t OggIterator::readAll(char *buffer, std::size_t max) { auto bytesRead = std::size_t(0); while (*this && max) { - const auto available = currentSegmentSize() - m_bytesRead; + const auto available = remainingBytesInCurrentSegment(); stream().seekg(static_cast(currentCharacterOffset()), std::ios_base::beg); if (max <= available) { stream().read(buffer + bytesRead, static_cast(max)); diff --git a/ogg/oggiterator.h b/ogg/oggiterator.h index 38ce0d1..e8b1b1a 100644 --- a/ogg/oggiterator.h +++ b/ogg/oggiterator.h @@ -34,6 +34,8 @@ public: std::uint64_t currentCharacterOffset() const; std::uint64_t tellg() const; std::uint32_t currentSegmentSize() const; + std::uint64_t remainingBytesInCurrentSegment() const; + std::uint64_t bytesReadFromCurrentSegment() const; void setFilter(std::uint32_t streamSerialId); void removeFilter(); bool isLastPageFetched() const; @@ -240,6 +242,22 @@ inline std::uint32_t OggIterator::currentSegmentSize() const return m_pages[m_page].segmentSizes()[m_segment]; } +/*! + * \brief Returns the number of bytes left to read in the current segment. + */ +inline std::uint64_t OggIterator::remainingBytesInCurrentSegment() const +{ + return currentSegmentSize() - m_bytesRead; +} + +/*! + * \brief Returns the number of bytes read from the current segment. + */ +inline uint64_t OggIterator::bytesReadFromCurrentSegment() const +{ + return m_bytesRead; +} + /*! * \brief Allows to filter pages by the specified \a streamSerialId. * diff --git a/ogg/oggpage.cpp b/ogg/oggpage.cpp index 95ffe4a..32eb404 100644 --- a/ogg/oggpage.cpp +++ b/ogg/oggpage.cpp @@ -5,6 +5,8 @@ #include #include +#include + using namespace std; using namespace CppUtilities; @@ -51,13 +53,15 @@ void OggPage::parseHeader(istream &stream, std::uint64_t startOffset, std::int32 maxSize -= m_segmentCount; } // read segment size table - m_segmentSizes.push_back(0); + m_segmentSizes.emplace_back(0); for (std::uint8_t i = 0; i < m_segmentCount;) { std::uint8_t entry = reader.readByte(); maxSize -= entry; m_segmentSizes.back() += entry; - if (++i < m_segmentCount && entry < 0xff) { - m_segmentSizes.push_back(0); + if (++i < m_segmentCount && entry < 0xFF) { + m_segmentSizes.emplace_back(0); + } else if (i == m_segmentCount && entry == 0xFF) { + m_headerTypeFlag |= 0x80; // FIXME v11: don't abuse header type flags } } // check whether the maximum size is exceeded diff --git a/ogg/oggpage.h b/ogg/oggpage.h index 807aaa6..cc9b721 100644 --- a/ogg/oggpage.h +++ b/ogg/oggpage.h @@ -25,6 +25,7 @@ public: bool isContinued() const; bool isFirstpage() const; bool isLastPage() const; + bool isLastSegmentUnconcluded() const; std::uint64_t absoluteGranulePosition() const; std::uint32_t streamSerialNumber() const; bool matchesStreamSerialNumber(std::uint32_t streamSerialNumber) const; @@ -101,7 +102,7 @@ inline std::uint8_t OggPage::streamStructureVersion() const */ inline std::uint8_t OggPage::headerTypeFlag() const { - return m_headerTypeFlag; + return m_headerTypeFlag & 0xF; // last 4 bits are used internally } /*! @@ -128,6 +129,14 @@ inline bool OggPage::isLastPage() const return m_headerTypeFlag & 0x04; } +/*! + * \brief Returns whether the last segment is unconcluded (the last lacing value of the last segment is 0xFF). + */ +inline bool OggPage::isLastSegmentUnconcluded() const +{ + return m_headerTypeFlag & 0x80; +} + /*! * \brief Returns the absolute granule position. * diff --git a/vorbis/vorbiscomment.cpp b/vorbis/vorbiscomment.cpp index 65c4ac8..5d23b34 100644 --- a/vorbis/vorbiscomment.cpp +++ b/vorbis/vorbiscomment.cpp @@ -6,6 +6,7 @@ #include "../diagnostics.h" #include "../exceptions.h" +#include #include #include #include @@ -201,6 +202,23 @@ template void VorbisComment::internalParse(StreamType &stream diag.emplace_back(DiagLevel::Critical, "Vorbis comment is truncated.", context); throw; } + + // warn if there are bytes left in the last segment of the Ogg packet containing the comment + if constexpr (std::is_same_v, OggIterator>) { + auto bytesRemaining = std::uint64_t(); + if (stream) { + bytesRemaining = stream.remainingBytesInCurrentSegment(); + if (stream.currentPage().isLastSegmentUnconcluded()) { + stream.nextSegment(); + if (stream) { + bytesRemaining += stream.remainingBytesInCurrentSegment(); + } + } + } + if (bytesRemaining) { + diag.emplace_back(DiagLevel::Warning, argsToString(bytesRemaining, " bytes left in last segment."), context); + } + } } /*! diff --git a/vorbis/vorbiscommentfield.cpp b/vorbis/vorbiscommentfield.cpp index a211d80..3931f57 100644 --- a/vorbis/vorbiscommentfield.cpp +++ b/vorbis/vorbiscommentfield.cpp @@ -52,7 +52,7 @@ template void VorbisCommentField::internalParse(StreamType &s static const string context("parsing Vorbis comment field"); char buff[4]; if (maxSize < 4) { - diag.emplace_back(DiagLevel::Critical, "Field expected.", context); + diag.emplace_back(DiagLevel::Critical, argsToString("Field expected at ", static_cast(stream.tellg()), '.'), context); throw TruncatedDataException(); } else { maxSize -= 4; @@ -71,7 +71,8 @@ template void VorbisCommentField::internalParse(StreamType &s setId(string(data.get(), idSize)); if (!idSize) { // empty field ID - diag.emplace_back(DiagLevel::Critical, "The field ID is empty.", context); + diag.emplace_back( + DiagLevel::Critical, argsToString("The field ID at ", static_cast(stream.tellg()), " is empty."), context); throw InvalidDataException(); } else if (id() == VorbisCommentIds::cover()) { // extract cover value @@ -99,7 +100,7 @@ template void VorbisCommentField::internalParse(StreamType &s setValue(TagValue(string(data.get() + idSize + 1, size - idSize - 1), TagTextEncoding::Utf8)); } } else { - diag.emplace_back(DiagLevel::Critical, "Field is truncated.", context); + diag.emplace_back(DiagLevel::Critical, argsToString("Field at ", static_cast(stream.tellg()), " is truncated."), context); throw TruncatedDataException(); } }