Git commit 7297f513388aac4b1239db5f832b7398158a2823 by Andreas Hartmetz. Committed on 03/12/2016 at 17:45. Pushed by ahartmetz into branch 'master'. Fix missing checks when not writing a new part of the signature. Among other things, the signature position when writing an array or dict would not get rewound to the start when it should. That was an over-eager optimization at work. I found the bug while working on a not yet pushed commit but let's get the fix in first. M +39 -39 serialization/arguments.cpp M +51 -0 tests/serialization/tst_arguments.cpp https://commits.kde.org/dferry/7297f513388aac4b1239db5f832b7398158a2823 diff --git a/serialization/arguments.cpp b/serialization/arguments.cpp index 6fc007b..9da1408 100644 --- a/serialization/arguments.cpp +++ b/serialization/arguments.cpp @@ -2159,47 +2159,47 @@ void Arguments::Writer::advanceState(cstring signat= ureFragment, IoState newState // signature additions must conform to syntax VALID_IF(d->m_signaturePosition + signatureFragment.length <=3D Ma= xSignatureLength, Error::SignatureTooLong); + } = - if (!d->m_aggregateStack.empty()) { - const Private::AggregateInfo &aggregateInfo =3D d->m_aggregate= Stack.back(); - switch (aggregateInfo.aggregateType) { - case BeginVariant: - // arrays and variants may contain just one single complet= e type; note that this will - // trigger only when not inside an aggregate inside the va= riant or (see below) array - if (d->m_signaturePosition >=3D 1) { - VALID_IF(newState =3D=3D EndVariant, Error::NotSingleC= ompleteTypeInVariant); - } - break; - case BeginArray: - if (d->m_signaturePosition >=3D aggregateInfo.arr.containe= dTypeBegin + 1 - && newState !=3D EndArray) { - // we are not at start of contained type's signature, = the array is at top of stack - // -> we are at the end of the single complete type in= side the array, start the next - // entry. TODO: check compatibility (essentially what'= s in the else branch below) - d->m_signaturePosition =3D aggregateInfo.arr.contained= TypeBegin; - isWritingSignature =3D false; - } - break; - case BeginDict: - if (d->m_signaturePosition =3D=3D aggregateInfo.arr.contai= nedTypeBegin) { - VALID_IF(isPrimitiveType || isStringType, Error::Inval= idKeyTypeInDict); - } - // first type has been checked already, second must be pre= sent (checked in EndDict - // state handler). no third type allowed. - if (d->m_signaturePosition >=3D aggregateInfo.arr.containe= dTypeBegin + 2 - && newState !=3D EndDict) { - // Start the next dict entry - d->m_nesting.parenCount +=3D 1; - // align to dict entry - d->m_elements.push_back(Private::ElementInfo(structAli= gnment, 0)); - d->m_signaturePosition =3D aggregateInfo.arr.contained= TypeBegin; - isWritingSignature =3D false; - m_state =3D DictKey; - } - break; - default: - break; + if (!d->m_aggregateStack.empty()) { + const Private::AggregateInfo &aggregateInfo =3D d->m_aggregateStac= k.back(); + switch (aggregateInfo.aggregateType) { + case BeginVariant: + // arrays and variants may contain just one single complete ty= pe; note that this will + // trigger only when not inside an aggregate inside the varian= t or (see below) array + if (d->m_signaturePosition >=3D 1) { + VALID_IF(newState =3D=3D EndVariant, Error::NotSingleCompl= eteTypeInVariant); + } + break; + case BeginArray: + if (d->m_signaturePosition >=3D aggregateInfo.arr.containedTyp= eBegin + 1 + && newState !=3D EndArray) { + // we are not at start of contained type's signature, the = array is at top of stack + // -> we are at the end of the single complete type inside= the array, start the next + // entry. TODO: check compatibility (essentially what's in= the else branch below) + d->m_signaturePosition =3D aggregateInfo.arr.containedType= Begin; + isWritingSignature =3D false; + } + break; + case BeginDict: + if (d->m_signaturePosition =3D=3D aggregateInfo.arr.containedT= ypeBegin) { + VALID_IF(isPrimitiveType || isStringType, Error::InvalidKe= yTypeInDict); + } + // first type has been checked already, second must be present= (checked in EndDict + // state handler). no third type allowed. + if (d->m_signaturePosition >=3D aggregateInfo.arr.containedTyp= eBegin + 2 + && newState !=3D EndDict) { + // Start the next dict entry + d->m_nesting.parenCount +=3D 1; + // align to dict entry + d->m_elements.push_back(Private::ElementInfo(structAlignme= nt, 0)); + d->m_signaturePosition =3D aggregateInfo.arr.containedType= Begin; + isWritingSignature =3D false; + m_state =3D DictKey; } + break; + default: + break; } } = diff --git a/tests/serialization/tst_arguments.cpp b/tests/serialization/ts= t_arguments.cpp index 201c737..9771a2b 100644 --- a/tests/serialization/tst_arguments.cpp +++ b/tests/serialization/tst_arguments.cpp @@ -1163,6 +1163,56 @@ static void test_realMessage() doRoundtrip(arg); } = +static void test_isWritingSignatureBug() +{ + { + // This was the original test, so it's the one with the comments :) + Arguments::Writer writer; + writer.beginArray(); + writer.beginStruct(); + writer.beginDict(); + writer.writeByte(1); + writer.writeByte(2); + writer.endDict(); + // Must add more stuff after the inner dict to ensure that= the signature position of the + // dict's value is well inside the existing signature in t= he second dict entry. + // See isWritingSignature in Writer::advanceState(). + writer.writeUint16(1); + writer.writeUint16(2); + writer.endStruct(); + writer.beginStruct(); + writer.beginDict(); + writer.writeByte(1); + writer.writeByte(2); + // In the second pass, we are definitely NOT writing a= new part of the dict signature, + // which used to go (that was the bug!!) through a dif= ferent code path in + // Arguments::Writer::advanceState(). + writer.writeByte(1); + TEST(writer.state() !=3D Arguments::InvalidData); + writer.writeUint16(2); + TEST(writer.state() =3D=3D Arguments::InvalidData); + } + { + // For completeness, do the equivalent of the previous test with a= n array inside + Arguments::Writer writer; + writer.beginArray(); + writer.beginStruct(); + writer.beginArray(); + writer.writeByte(1); + writer.endArray(); + writer.writeUint16(1); + writer.writeUint16(2); + writer.endStruct(); + writer.beginStruct(); + writer.beginArray(); + writer.writeByte(1); + writer.writeByte(1); + TEST(writer.state() !=3D Arguments::InvalidData); + writer.writeUint16(2); + TEST(writer.state() =3D=3D Arguments::InvalidData); + } +} + static void writeValue(Arguments::Writer *writer, uint32 typeIndex, const = void *value) { switch (typeIndex) { @@ -1679,6 +1729,7 @@ int main(int, char *[]) test_alignment(); test_arrayOfVariant(); test_realMessage(); + test_isWritingSignatureBug(); test_primitiveArray(); test_signatureLengths(); test_emptyArrayAndDict();