[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    [dferry] /: Fix a spec violation regarding alignment in empty arrays.
From:       Andreas Hartmetz <ahartmetz () gmail ! com>
Date:       2016-12-03 17:45:33
Message-ID: E1cDENR-0001Db-M1 () code ! kde ! org
[Download RAW message or body]

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 = firstElementTy.state() == BeginDict ? BeginDict : BeginArray;
 
         uint32 dataEnd = d->m_dataPosition;
-        if (likely(arrayLength)) {
+        // If the internal type has greater alignment requirements than the array \
index type (which has +        // 4 bytes), align to the nonexistent first element.
+        // Tricky: d->m_nilArrayNesting is only increased when the API client calls \
beginArray(), so this +        // is the old state. Use it to increase alignment \
requirements once. If my guess from the previous +        // paragraph was wrong, \
don't look at d->m_nilArrayNesting and just apply the alignment. +        if \
(likely(!d->m_nilArrayNesting)) {  const uint32 padStart = d->m_dataPosition;
             d->m_dataPosition = 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 signatureFragment, 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 alignment of \
their contents +            // expects *some* kind of data element in the array. \
Since a string after an array length +            // field can never change the \
alignment, just add something that does nothing. +            \
d->m_elements.push_back(Private::ElementInfo(1, 0));  }
         return;
     }
@@ -2342,9 +2352,13 @@ void Arguments::Writer::advanceState(cstring \
signatureFragment, IoState newState  Error::TooFewTypesInArrayOrDict);
         d->m_aggregateStack.pop_back();
         if (unlikely(d->m_nilArrayNesting)) {
-            if (!--d->m_nilArrayNesting) {
+            if (--d->m_nilArrayNesting == 0) {
+                assert(d->m_elements.begin() + d->m_dataElementsCountBeforeNilArray \
                <= d->m_elements.end());
                 d->m_elements.erase(d->m_elements.begin() + \
d->m_dataElementsCountBeforeNilArray,  d->m_elements.end());
+                assert((d->m_elements.end() - 2)->size == \
Private::ElementInfo::ArrayLengthField); +                // align, but don't have \
actual data for the first element +                d->m_elements.back().size = 0;
                 d->m_dataPosition = d->m_dataPositionBeforeNilArray;
             }
         }
@@ -2374,7 +2388,7 @@ void Arguments::Writer::beginArrayOrDict(IoState beginWhat, \
                ArrayOption option)
                     // The code is a slightly modified version of code below under: \
if (isEmpty) {  if (!d->m_nilArrayNesting) {
                         d->m_nilArrayNesting = 1;
-                        d->m_dataElementsCountBeforeNilArray = d->m_elements.size() \
+ 1; // as below +                        d->m_dataElementsCountBeforeNilArray = \
                d->m_elements.size() + 2; // +2 as below
                         // Now correct for the elements already added in \
                advanceState() with BeginArray / BeginDict
                         d->m_dataElementsCountBeforeNilArray -= (beginWhat == \
                BeginDict) ? 2 : 1;
                         d->m_dataPositionBeforeNilArray = d->m_dataPosition;
@@ -2396,7 +2410,8 @@ void Arguments::Writer::beginArrayOrDict(IoState beginWhat, \
                ArrayOption option)
             // variant signatures written inside an empty array. When we close the \
                array, though, we
             // throw away all that data and signatures and keep only changes in the \
signature containing  // the topmost empty array.
-            d->m_dataElementsCountBeforeNilArray = d->m_elements.size() + 1; // +1 \
-> keep ArrayLengthField +            // +2 -> keep ArrayLengthField, and first data \
element for alignment purposes +            d->m_dataElementsCountBeforeNilArray = \
d->m_elements.size() + 2;  d->m_dataPositionBeforeNilArray = d->m_dataPosition;
         }
     }
@@ -2604,6 +2619,7 @@ void Arguments::Writer::finishInternal()
                     al.lengthFieldPosition = bufferPos;
                     bufferPos += sizeof(uint32);
                     // alignment padding before first array element in output
+                    assert(i + 1 < d->m_elements.size());
                     zeroPad(buffer, d->m_elements[i + 1].alignment(), &bufferPos);
                     // array data starts at the first array element position
                     al.dataStartPosition = bufferPos;
diff --git a/tests/serialization/tst_arguments.cpp \
b/tests/serialization/tst_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, \
sizeof(testData)))); +    }
+    {
+        // Spec says: alignment padding after array length, even if the array \
contains no data. Test this +        // with different types and alignment \
situations. +        byte testData[40] = {
+            0, 0, 0, 0, // length of array of uint64s - zero
+            0, 0, 0, 0, // alignment padding to 8 bytes (= natural alignment 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 (= 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 alignment 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 (= alignment of struct)
+        };
+        doRoundtrip(Arguments(nullptr, cstring("atuaxa{uu}yyyyya(u)"), \
chunk(testData, sizeof(testData))));  }
 }
 


[prev in list] [next in list] [prev in thread] [next in thread]