ogg vorbis corruption

[thepaka]

Hello,
I'm trying to debug an ogg vorbis corruption that happens when editing tags with taglib. And I just found something suspicious in Ogg::File::writePacket .

Seems that if lastPage has 2 packets, the beginning of the second will be lost because the if condition is true only if lastPage has 3 packets or more.

diff --git a/taglib/ogg/oggfile.cpp b/taglib/ogg/oggfile.cpp
index 86b0b07..c36e4d4 100644
--- a/taglib/ogg/oggfile.cpp
+++ b/taglib/ogg/oggfile.cpp
@@ -253,7 +253,7 @@ void Ogg::File::writePacket(unsigned int i, const ByteVector &packet)
   ByteVectorList packets = firstPage->packets();
   packets[i - firstPage->firstPacketIndex()] = packet;
 
-  if(firstPage != lastPage && lastPage->packetCount() > 2) {
+  if(firstPage != lastPage && lastPage->packetCount() > 1) {
     ByteVectorList lastPagePackets = lastPage->packets();
     lastPagePackets.erase(lastPagePackets.begin());
     packets.append(lastPagePackets);

This change fixed the corruption issue in my test case (using vorbiscomment and tagwriter). I will do more tests in the next days.

[TsudaKageyu]

@thepaka, thanks for your report.
I will do some tests on my side. However, I'm looking forward to your next report.

[thepaka]

Initially, I got corruptions while playing them in Amarok (It use taglib to update the playcount and score tag after playing them). Since I use my fixed version of taglib I have no new corrupted files.

Here is how I reproduce the corruption with the current version of taglib (1.11.1)

# 'ogginfo' and 'vorbiscomment' belongs to 'vorbis-tools'
# 'tagwriter' is taglib-1.11.1/examples/tagwriter.cpp

# define your test file
ogg_orig="/tmp/taglib/orig.ogg" 
ogg="/tmp/taglib/test.ogg"
cp "$ogg_orig" "$ogg" 

# make a big string
tagsize="64963"
tag="T=$(printf '%*s' $(( tagsize-2 )) | tr ' ' 'a')"

# check if the test file is corrupted (should not)
ogginfo -q -q "$ogg" || echo "corrupted"

# replace all tags with a big tag string without using taglib
vorbiscomment -w -t "$tag" "$ogg"

# check if the test file is corrupted (should not)
ogginfo -q -q "$ogg" || echo "corrupted"

# use taglib to rewrite the test file
# removing an inexistent tag will just make taglib parse and rewrite the file
./tagwriter -D "foo" "$ogg"

# check if the test file is corrupted (it should be now)
ogginfo -q -q "$ogg" || echo "corrupted"

Note: In my tests, for the tagsize value, I found that the corruption happens starting to 64963.

[TsudaKageyu]

Thanks. I'm trying to write a test to reproduce it.

[TsudaKageyu]

I found another bug when investigating this one. I will fix them soon.

[TsudaKageyu]

Hi @thepaka,
I think we have fixed the bug. Could you try our latest code?

[thepaka]

Hi, no corruption since I use the 489e2e6 version (1 Dec). I think its fixed.
Thank you.

[TsudaKageyu]

Thanks!