[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-commits
Subject: [dferry] /: Fix missing checks when not writing a new part of the signature.
From: Andreas Hartmetz <ahartmetz () gmail ! com>
Date: 2016-12-03 17:47:12
Message-ID: E1cDEP2-0002tJ-DV () code ! kde ! org
[Download RAW message or body]
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 \
signatureFragment, IoState newState // signature additions must conform to syntax
VALID_IF(d->m_signaturePosition + signatureFragment.length <= \
MaxSignatureLength, Error::SignatureTooLong);
+ }
- if (!d->m_aggregateStack.empty()) {
- const Private::AggregateInfo &aggregateInfo = \
d->m_aggregateStack.back();
- switch (aggregateInfo.aggregateType) {
- case BeginVariant:
- // arrays and variants may contain just one single complete type; \
note that this will
- // trigger only when not inside an aggregate inside the variant or \
(see below) array
- if (d->m_signaturePosition >= 1) {
- VALID_IF(newState == EndVariant, \
Error::NotSingleCompleteTypeInVariant);
- }
- break;
- case BeginArray:
- if (d->m_signaturePosition >= aggregateInfo.arr.containedTypeBegin + \
1
- && newState != 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 = aggregateInfo.arr.containedTypeBegin;
- isWritingSignature = false;
- }
- break;
- case BeginDict:
- if (d->m_signaturePosition == aggregateInfo.arr.containedTypeBegin) \
{
- VALID_IF(isPrimitiveType || isStringType, \
Error::InvalidKeyTypeInDict);
- }
- // first type has been checked already, second must be present \
(checked in EndDict
- // state handler). no third type allowed.
- if (d->m_signaturePosition >= aggregateInfo.arr.containedTypeBegin + \
2
- && newState != EndDict) {
- // Start the next dict entry
- d->m_nesting.parenCount += 1;
- // align to dict entry
- d->m_elements.push_back(Private::ElementInfo(structAlignment, \
0));
- d->m_signaturePosition = aggregateInfo.arr.containedTypeBegin;
- isWritingSignature = false;
- m_state = DictKey;
- }
- break;
- default:
- break;
+ if (!d->m_aggregateStack.empty()) {
+ const Private::AggregateInfo &aggregateInfo = d->m_aggregateStack.back();
+ switch (aggregateInfo.aggregateType) {
+ case BeginVariant:
+ // arrays and variants may contain just one single complete type; note \
that this will + // trigger only when not inside an aggregate inside the \
variant or (see below) array + if (d->m_signaturePosition >= 1) {
+ VALID_IF(newState == EndVariant, \
Error::NotSingleCompleteTypeInVariant); + }
+ break;
+ case BeginArray:
+ if (d->m_signaturePosition >= aggregateInfo.arr.containedTypeBegin + 1
+ && newState != 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 \
= aggregateInfo.arr.containedTypeBegin; + isWritingSignature = false;
+ }
+ break;
+ case BeginDict:
+ if (d->m_signaturePosition == aggregateInfo.arr.containedTypeBegin) {
+ VALID_IF(isPrimitiveType || isStringType, \
Error::InvalidKeyTypeInDict); + }
+ // first type has been checked already, second must be present (checked \
in EndDict + // state handler). no third type allowed.
+ if (d->m_signaturePosition >= aggregateInfo.arr.containedTypeBegin + 2
+ && newState != EndDict) {
+ // Start the next dict entry
+ d->m_nesting.parenCount += 1;
+ // align to dict entry
+ d->m_elements.push_back(Private::ElementInfo(structAlignment, 0));
+ d->m_signaturePosition = aggregateInfo.arr.containedTypeBegin;
+ isWritingSignature = false;
+ m_state = DictKey;
}
+ break;
+ default:
+ break;
}
}
diff --git a/tests/serialization/tst_arguments.cpp \
b/tests/serialization/tst_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 the 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 different code path in + // \
Arguments::Writer::advanceState(). + writer.writeByte(1);
+ TEST(writer.state() != Arguments::InvalidData);
+ writer.writeUint16(2);
+ TEST(writer.state() == Arguments::InvalidData);
+ }
+ {
+ // For completeness, do the equivalent of the previous test with an 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() != Arguments::InvalidData);
+ writer.writeUint16(2);
+ TEST(writer.state() == 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();
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic