Improve warnings when parsing Vorbis comments

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<std::streamoff>(currentCharacterOffset()));
         if (count <= available) {
             stream().read(buffer + bytesRead, static_cast<std::streamsize>(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<std::streamoff>(currentCharacterOffset()), std::ios_base::beg);
         if (max <= available) {
             stream().read(buffer + bytesRead, static_cast<std::streamsize>(max));

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.
  *

ogg/oggpage.cpp

@@ -5,6 +5,8 @@
 #include <c++utilities/conversion/binaryconversion.h>
 #include <c++utilities/io/binaryreader.h>
 
+#include <limits>
+
 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

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.
  *

vorbis/vorbiscomment.cpp

@@ -6,6 +6,7 @@
 #include "../diagnostics.h"
 #include "../exceptions.h"
 
+#include <c++utilities/conversion/stringbuilder.h>
 #include <c++utilities/io/binaryreader.h>
 #include <c++utilities/io/binarywriter.h>
 #include <c++utilities/io/copy.h>
@@ -201,6 +202,23 @@ template <class StreamType> 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<std::decay_t<StreamType>, 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);
+        }
+    }
 }
 
 /*!

vorbis/vorbiscommentfield.cpp

@@ -52,7 +52,7 @@ template <class StreamType> 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<std::streamoff>(stream.tellg()), '.'), context);
         throw TruncatedDataException();
     } else {
         maxSize -= 4;
@@ -71,7 +71,8 @@ template <class StreamType> 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<std::streamoff>(stream.tellg()), " is empty."), context);
                 throw InvalidDataException();
             } else if (id() == VorbisCommentIds::cover()) {
                 // extract cover value
@@ -99,7 +100,7 @@ template <class StreamType> 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<std::streamoff>(stream.tellg()), " is truncated."), context);
             throw TruncatedDataException();
         }
     }