Git commit f4bca438b0f0dc5af1092197a5aacc373d6a8900 by Andreas Hartmetz. Committed on 03/12/2016 at 17:45. Pushed by ahartmetz into branch 'master'. Fix a spec violation regarding alignment in empty arrays. The spec says that alignment padding to the element type follows the array length field. It doesn't mention the zero elements case explicitly, but evidently (the daemon throws out our messages otherwise...) the alignment is required in all cases. M +20 -4 serialization/arguments.cpp M +22 -1 tests/serialization/tst_arguments.cpp https://commits.kde.org/dferry/f4bca438b0f0dc5af1092197a5aacc373d6a8900 diff --git a/serialization/arguments.cpp b/serialization/arguments.cpp index 4bae63b..6fc007b 100644 --- a/serialization/arguments.cpp +++ b/serialization/arguments.cpp @@ -1418,7 +1418,12 @@ void Arguments::Reader::advanceState() m_state =3D firstElementTy.state() =3D=3D BeginDict ? BeginDict : = BeginArray; = uint32 dataEnd =3D d->m_dataPosition; - if (likely(arrayLength)) { + // If the internal type has greater alignment requirements than th= e array index type (which has + // 4 bytes), align to the nonexistent first element. + // Tricky: d->m_nilArrayNesting is only increased when the API cli= ent calls beginArray(), so this + // is the old state. Use it to increase alignment requirements onc= e. If my guess from the previous + // paragraph was wrong, don't look at d->m_nilArrayNesting and jus= t apply the alignment. + if (likely(!d->m_nilArrayNesting)) { const uint32 padStart =3D d->m_dataPosition; d->m_dataPosition =3D align(d->m_dataPosition, firstElementTy.= alignment); VALID_IF(isPaddingZero(d->m_data, padStart, d->m_dataPosition)= , Error::MalformedMessageData); @@ -2236,6 +2241,11 @@ void Arguments::Writer::advanceState(cstring signatu= reFragment, IoState newState // allowed to be garbage) is not validated and no wild pointer is = dereferenced. if (likely(!d->m_nilArrayNesting)) { doWriteString(newState, alignment); + } else { + // The code that ensures that even empty arrays respect the al= ignment of their contents + // expects *some* kind of data element in the array. Since a s= tring after an array length + // field can never change the alignment, just add something th= at does nothing. + d->m_elements.push_back(Private::ElementInfo(1, 0)); } return; } @@ -2342,9 +2352,13 @@ void Arguments::Writer::advanceState(cstring signatu= reFragment, IoState newState Error::TooFewTypesInArrayOrDict); d->m_aggregateStack.pop_back(); if (unlikely(d->m_nilArrayNesting)) { - if (!--d->m_nilArrayNesting) { + if (--d->m_nilArrayNesting =3D=3D 0) { + assert(d->m_elements.begin() + d->m_dataElementsCountBefor= eNilArray <=3D d->m_elements.end()); d->m_elements.erase(d->m_elements.begin() + d->m_dataEleme= ntsCountBeforeNilArray, d->m_elements.end()); + assert((d->m_elements.end() - 2)->size =3D=3D Private::Ele= mentInfo::ArrayLengthField); + // align, but don't have actual data for the first element + d->m_elements.back().size =3D 0; d->m_dataPosition =3D d->m_dataPositionBeforeNilArray; } } @@ -2374,7 +2388,7 @@ void Arguments::Writer::beginArrayOrDict(IoState begi= nWhat, ArrayOption option) // The code is a slightly modified version of code bel= ow under: if (isEmpty) { if (!d->m_nilArrayNesting) { d->m_nilArrayNesting =3D 1; - d->m_dataElementsCountBeforeNilArray =3D d->m_elem= ents.size() + 1; // as below + d->m_dataElementsCountBeforeNilArray =3D d->m_elem= ents.size() + 2; // +2 as below // Now correct for the elements already added in a= dvanceState() with BeginArray / BeginDict d->m_dataElementsCountBeforeNilArray -=3D (beginWh= at =3D=3D BeginDict) ? 2 : 1; d->m_dataPositionBeforeNilArray =3D d->m_dataPosit= ion; @@ -2396,7 +2410,8 @@ void Arguments::Writer::beginArrayOrDict(IoState begi= nWhat, ArrayOption option) // variant signatures written inside an empty array. When we c= lose the array, though, we // throw away all that data and signatures and keep only chang= es in the signature containing // the topmost empty array. - d->m_dataElementsCountBeforeNilArray =3D d->m_elements.size() = + 1; // +1 -> keep ArrayLengthField + // +2 -> keep ArrayLengthField, and first data element for ali= gnment purposes + d->m_dataElementsCountBeforeNilArray =3D d->m_elements.size() = + 2; d->m_dataPositionBeforeNilArray =3D d->m_dataPosition; } } @@ -2604,6 +2619,7 @@ void Arguments::Writer::finishInternal() al.lengthFieldPosition =3D bufferPos; bufferPos +=3D sizeof(uint32); // alignment padding before first array element in out= put + assert(i + 1 < d->m_elements.size()); zeroPad(buffer, d->m_elements[i + 1].alignment(), &buf= ferPos); // array data starts at the first array element positi= on al.dataStartPosition =3D bufferPos; diff --git a/tests/serialization/tst_arguments.cpp b/tests/serialization/ts= t_arguments.cpp index d2dabf9..201c737 100644 --- a/tests/serialization/tst_arguments.cpp +++ b/tests/serialization/tst_arguments.cpp @@ -806,7 +806,28 @@ static void test_roundtrip() 1, 2, 3, 4, 5, 6, 7, 8, // the double 20, 21, 22, 23 // the int (not part of the variant) }; - doRoundtrip(Arguments(nullptr, cstring("vi"), chunk(testData, 36))= ); + doRoundtrip(Arguments(nullptr, cstring("vi"), chunk(testData, size= of(testData)))); + } + { + // Spec says: alignment padding after array length, even if the ar= ray contains no data. Test this + // with different types and alignment situations. + byte testData[40] =3D { + 0, 0, 0, 0, // length of array of uint64s - zero + 0, 0, 0, 0, // alignment padding to 8 bytes (=3D natural align= ment of uint64) + // ... zero uint64s ... + 1, 2, 3, 4, // a uint32 to change the alignment, just to test + 0, 0, 0, 0, // length of array of int64s - zero + // no alignment padding needed here + 0, 0, 0, 0, // length of dict {uint32, uint32} - zero + 0, 0, 0, 0, // alignment padding to 8 bytes (=3D alignment of = dict entry) + // some data (single bytes) between the arrays to prevent all = those zeros from accidentally + // looking valid when the Reader is confused. Also upset the a= lignment a bit. + 101, 102, 103, 104, 105, + 0, 0, 0, // padding to alignment of array size + 0, 0, 0, 0, // length of array of structs - zero + 0, 0, 0, 0 // alignment padding to 8 bytes (=3D alignment of = struct) + }; + doRoundtrip(Arguments(nullptr, cstring("atuaxa{uu}yyyyya(u)"), chu= nk(testData, sizeof(testData)))); } } =20