From ff8443f33a4d6c7a577b66048692f0471df76830 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 2 May 2015 02:34:40 +0900 Subject: [PATCH 1/4] Fix the wrong padding of ByteVector::resize(). The expanded area will be filled with garbage instead of correct padding in some corner cases. --- taglib/toolkit/tbytevector.cpp | 1 + tests/test_bytevector.cpp | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/taglib/toolkit/tbytevector.cpp b/taglib/toolkit/tbytevector.cpp index 4ae40666..bf6c525f 100644 --- a/taglib/toolkit/tbytevector.cpp +++ b/taglib/toolkit/tbytevector.cpp @@ -702,6 +702,7 @@ ByteVector &ByteVector::resize(uint size, char padding) { if(size != d->length) { detach(); + d->data->data.resize(d->offset + d->length); d->data->data.resize(d->offset + size, padding); d->length = size; } diff --git a/tests/test_bytevector.cpp b/tests/test_bytevector.cpp index c68a8c34..e9d43c05 100644 --- a/tests/test_bytevector.cpp +++ b/tests/test_bytevector.cpp @@ -123,6 +123,12 @@ public: CPPUNIT_ASSERT(i.containsAt(j, 5, 0)); CPPUNIT_ASSERT(i.containsAt(j, 6, 1)); CPPUNIT_ASSERT(i.containsAt(j, 6, 1, 3)); + + ByteVector k = ByteVector("0123456789").mid(3, 4); + k.resize(6, 'A'); + CPPUNIT_ASSERT_EQUAL(uint(6), k.size()); + CPPUNIT_ASSERT_EQUAL('6', k[3]); + CPPUNIT_ASSERT_EQUAL('A', k[4]); } void testFind1() From a924ca0db76079e3d95e885afa2eca4822eaa0d6 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 14 May 2015 11:20:35 +0900 Subject: [PATCH 2/4] Expand the internal buffer of ByteVector only if really needed. Add tests for all execution paths of ByteVector::resize(). --- taglib/toolkit/tbytevector.cpp | 9 ++++-- tests/test_bytevector.cpp | 51 ++++++++++++++++++++++++++++++---- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/taglib/toolkit/tbytevector.cpp b/taglib/toolkit/tbytevector.cpp index bf6c525f..7aeaea92 100644 --- a/taglib/toolkit/tbytevector.cpp +++ b/taglib/toolkit/tbytevector.cpp @@ -702,8 +702,13 @@ ByteVector &ByteVector::resize(uint size, char padding) { if(size != d->length) { detach(); - d->data->data.resize(d->offset + d->length); - d->data->data.resize(d->offset + size, padding); + + if(size > d->data->data.size() - d->offset) + d->data->data.resize(d->offset + size); + + if(size > d->length) + ::memset(DATA(d) + d->offset + d->length, padding, size - d->length); + d->length = size; } diff --git a/tests/test_bytevector.cpp b/tests/test_bytevector.cpp index e9d43c05..de682084 100644 --- a/tests/test_bytevector.cpp +++ b/tests/test_bytevector.cpp @@ -42,6 +42,7 @@ class TestByteVector : public CppUnit::TestFixture CPPUNIT_TEST(testNumericCoversion); CPPUNIT_TEST(testReplace); CPPUNIT_TEST(testIterator); + CPPUNIT_TEST(testResize); CPPUNIT_TEST_SUITE_END(); public: @@ -123,12 +124,6 @@ public: CPPUNIT_ASSERT(i.containsAt(j, 5, 0)); CPPUNIT_ASSERT(i.containsAt(j, 6, 1)); CPPUNIT_ASSERT(i.containsAt(j, 6, 1, 3)); - - ByteVector k = ByteVector("0123456789").mid(3, 4); - k.resize(6, 'A'); - CPPUNIT_ASSERT_EQUAL(uint(6), k.size()); - CPPUNIT_ASSERT_EQUAL('6', k[3]); - CPPUNIT_ASSERT_EQUAL('A', k[4]); } void testFind1() @@ -340,6 +335,50 @@ public: CPPUNIT_ASSERT_EQUAL('3', *it4); } + void testResize() + { + ByteVector a = ByteVector("0123456789"); + ByteVector b = a.mid(3, 4); + b.resize(6, 'A'); + CPPUNIT_ASSERT_EQUAL(uint(6), b.size()); + CPPUNIT_ASSERT_EQUAL('6', b[3]); + CPPUNIT_ASSERT_EQUAL('A', b[4]); + CPPUNIT_ASSERT_EQUAL('A', b[5]); + b.resize(10, 'B'); + CPPUNIT_ASSERT_EQUAL(uint(10), b.size()); + CPPUNIT_ASSERT_EQUAL('6', b[3]); + CPPUNIT_ASSERT_EQUAL('B', b[6]); + CPPUNIT_ASSERT_EQUAL('B', b[9]); + b.resize(3, 'C'); + CPPUNIT_ASSERT_EQUAL(uint(3), b.size()); + CPPUNIT_ASSERT_EQUAL(-1, b.find('C')); + b.resize(3); + CPPUNIT_ASSERT_EQUAL(uint(3), b.size()); + + // Check if a and b were properly detached. + + CPPUNIT_ASSERT_EQUAL(uint(10), a.size()); + CPPUNIT_ASSERT_EQUAL('3', a[3]); + CPPUNIT_ASSERT_EQUAL('5', a[5]); + + // Special case that refCount == 1 and d->offset != 0. + + ByteVector c = ByteVector("0123456789").mid(3, 4); + c.resize(6, 'A'); + CPPUNIT_ASSERT_EQUAL(uint(6), c.size()); + CPPUNIT_ASSERT_EQUAL('6', c[3]); + CPPUNIT_ASSERT_EQUAL('A', c[4]); + CPPUNIT_ASSERT_EQUAL('A', c[5]); + c.resize(10, 'B'); + CPPUNIT_ASSERT_EQUAL(uint(10), c.size()); + CPPUNIT_ASSERT_EQUAL('6', c[3]); + CPPUNIT_ASSERT_EQUAL('B', c[6]); + CPPUNIT_ASSERT_EQUAL('B', c[9]); + c.resize(3, 'C'); + CPPUNIT_ASSERT_EQUAL(uint(3), c.size()); + CPPUNIT_ASSERT_EQUAL(-1, c.find('C')); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestByteVector); From 1f99c93a61085bbd625621cfe0c075bbc323b076 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 16 May 2015 03:46:34 +0900 Subject: [PATCH 3/4] Reduce redundant memset when resizing ByteVector. --- taglib/toolkit/tbytevector.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/taglib/toolkit/tbytevector.cpp b/taglib/toolkit/tbytevector.cpp index 7aeaea92..ad5d4523 100644 --- a/taglib/toolkit/tbytevector.cpp +++ b/taglib/toolkit/tbytevector.cpp @@ -703,11 +703,15 @@ ByteVector &ByteVector::resize(uint size, char padding) if(size != d->length) { detach(); - if(size > d->data->data.size() - d->offset) - d->data->data.resize(d->offset + size); + const size_t bufferSize = d->data->data.size(); - if(size > d->length) - ::memset(DATA(d) + d->offset + d->length, padding, size - d->length); + if(size > bufferSize - d->offset) { + d->data->data.resize(d->offset + size, padding); + ::memset(&*end(), padding, bufferSize - (d->length + d->offset)); + } + else if(size > d->length) { + ::memset(&*end(), padding, size - d->length); + } d->length = size; } From b021ed44e95647fb37ca8cc2cb312bd8a6ed9347 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 16 May 2015 11:16:00 +0900 Subject: [PATCH 4/4] Revert the last two commits. But leave the tests unchanged, and add some comments. --- taglib/toolkit/tbytevector.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/taglib/toolkit/tbytevector.cpp b/taglib/toolkit/tbytevector.cpp index ad5d4523..45ad0317 100644 --- a/taglib/toolkit/tbytevector.cpp +++ b/taglib/toolkit/tbytevector.cpp @@ -703,15 +703,12 @@ ByteVector &ByteVector::resize(uint size, char padding) if(size != d->length) { detach(); - const size_t bufferSize = d->data->data.size(); + // Remove the excessive length of the internal buffer first to pad correctly. + // This doesn't reallocate the buffer, since std::vector::resize() doesn't + // reallocate the buffer when shrinking. - if(size > bufferSize - d->offset) { - d->data->data.resize(d->offset + size, padding); - ::memset(&*end(), padding, bufferSize - (d->length + d->offset)); - } - else if(size > d->length) { - ::memset(&*end(), padding, size - d->length); - } + d->data->data.resize(d->offset + d->length); + d->data->data.resize(d->offset + size, padding); d->length = size; }