Skip to content

Commit 497066e

Browse files
thiagomacieirathiagomacieira
committed
Encoder: Fix off-by-one error encoding negative numbers
The documentation said we encoded the negative value equivalent to the passed absolute value. That means encode_number() requires the subtraction. This commit takes the opportunity to unit-test the rest of the integer API in the parser. Signed-off-by: Thiago Macieira <[email protected]>
1 parent 1b9a640 commit 497066e

File tree

3 files changed

+126
-5
lines changed

3 files changed

+126
-5
lines changed

‎src/cborencoder.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -332,11 +332,13 @@ CborError cbor_encode_uint(CborEncoder *encoder, uint64_t value)
332332
* Appends the negative 64-bit integer whose absolute value is \a
333333
* absolute_value to the CBOR stream provided by \a encoder.
334334
*
335+
* If the value \a absolute_value is zero, this function encodes -2^64 - 1.
336+
*
335337
* \sa cbor_encode_uint, cbor_encode_int
336338
*/
337339
CborError cbor_encode_negative_int(CborEncoder *encoder, uint64_t absolute_value)
338340
{
339-
return encode_number(encoder, absolute_value, NegativeIntegerType << MajorTypeShift);
341+
return encode_number(encoder, absolute_value - 1, NegativeIntegerType << MajorTypeShift);
340342
}
341343

342344
/**

‎tests/encoder/tst_encoder.cpp

+23-4
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,9 @@ template <size_t N> QByteArray raw(const char (&data)[N])
129129
return QByteArray::fromRawData(data, N - 1);
130130
}
131131

132+
struct NegativeInteger { quint64 abs; };
133+
Q_DECLARE_METATYPE(NegativeInteger)
134+
132135
struct SimpleType { uint8_t type; };
133136
Q_DECLARE_METATYPE(SimpleType)
134137

@@ -209,6 +212,8 @@ CborError encodeVariant(CborEncoder *encoder, const QVariant &v)
209212
}
210213

211214
default:
215+
if (type == qMetaTypeId<NegativeInteger>())
216+
return cbor_encode_negative_int(encoder, v.value<NegativeInteger>().abs);
212217
if (type == qMetaTypeId<SimpleType>())
213218
return cbor_encode_simple_value(encoder, v.value<SimpleType>().type);
214219
#if QT_VERSION < QT_VERSION_CHECK(5, 9, 0)
@@ -306,7 +311,7 @@ void addFixedData()
306311
QTest::newRow("UINT64_MAX") << raw("\x1b" "\xff\xff\xff\xff" "\xff\xff\xff\xff")
307312
<< QVariant(std::numeric_limits<quint64>::max());
308313

309-
// signed integers containing positive numbers
314+
// signed integers containing non-negative numbers
310315
QTest::newRow("0") << raw("\x00") << QVariant(0);
311316
QTest::newRow("1") << raw("\x01") << QVariant(1);
312317
QTest::newRow("10") << raw("\x0a") << QVariant(10);
@@ -319,7 +324,7 @@ void addFixedData()
319324
QTest::newRow("4294967295") << raw("\x1a\xff\xff\xff\xff") << QVariant(Q_INT64_C(4294967295));
320325
QTest::newRow("4294967296") << raw("\x1b\0\0\0\1\0\0\0\0") << QVariant(Q_INT64_C(4294967296));
321326

322-
// negative integers
327+
// signed integers containing negative numbers
323328
QTest::newRow("-1") << raw("\x20") << QVariant(-1);
324329
QTest::newRow("-2") << raw("\x21") << QVariant(-2);
325330
QTest::newRow("-24") << raw("\x37") << QVariant(-24);
@@ -330,8 +335,22 @@ void addFixedData()
330335
QTest::newRow("-UINT16_MAX-1") << raw("\x3a\0\1\x00\x00") << QVariant(-65537);
331336
QTest::newRow("-UINT32_MAX") << raw("\x3a\xff\xff\xff\xff") << QVariant(Q_INT64_C(-4294967296));
332337
QTest::newRow("-UINT32_MAX-1") << raw("\x3b\0\0\0\1\0\0\0\0") << QVariant(Q_INT64_C(-4294967297));
333-
// QTest::newRow("-UINT64_MAX") << raw("\x3b" "\xff\xff\xff\xff" "\xff\xff\xff\xff")
334-
// << QVariant::fromValue(BigNegative{std::numeric_limits<quint64>::max()});
338+
339+
// negative integers
340+
auto neg = [](quint64 v) { return QVariant::fromValue<NegativeInteger>({v}); };
341+
QTest::newRow("negative1") << raw("\x20") << neg(1);
342+
QTest::newRow("negative2") << raw("\x21") << neg(2);
343+
QTest::newRow("negative24") << raw("\x37") << neg(24);
344+
QTest::newRow("negative25") << raw("\x38\x18") << neg(25);
345+
QTest::newRow("negativeUINT8_MAX") << raw("\x38\xff") << neg(256);
346+
QTest::newRow("negativeUINT8_MAX-1") << raw("\x39\x01\x00") << neg(257);
347+
QTest::newRow("negativeUINT16_MAX") << raw("\x39\xff\xff") << neg(65536);
348+
QTest::newRow("negativeUINT16_MAX-1") << raw("\x3a\0\1\x00\x00") << neg(65537);
349+
QTest::newRow("negativeUINT32_MAX") << raw("\x3a\xff\xff\xff\xff") << neg(Q_UINT64_C(4294967296));
350+
QTest::newRow("negativeUINT32_MAX-1") << raw("\x3b\0\0\0\1\0\0\0\0") << neg(Q_UINT64_C(4294967297));
351+
QTest::newRow("negativeUINT64_MAX") << raw("\x3b" "\xff\xff\xff\xff" "\xff\xff\xff\xfe")
352+
<< neg(std::numeric_limits<quint64>::max());
353+
QTest::newRow("negativeUINT64_MAX+1") << raw("\x3b" "\xff\xff\xff\xff" "\xff\xff\xff\xff") << neg(0);
335354

336355
QTest::newRow("simple0") << raw("\xe0") << QVariant::fromValue(SimpleType{0});
337356
QTest::newRow("simple19") << raw("\xf3") << QVariant::fromValue(SimpleType{19});

‎tests/parser/tst_parser.cpp

+100
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ private slots:
4343
void initParserEmpty();
4444

4545
// parsing API
46+
void integers_data();
47+
void integers();
4648
void fixed_data();
4749
void fixed();
4850
void strings_data();
@@ -322,6 +324,104 @@ void addFixedData()
322324

323325
}
324326

327+
static void addIntegers()
328+
{
329+
QTest::addColumn<QByteArray>("data");
330+
QTest::addColumn<quint64>("expectedRaw");
331+
QTest::addColumn<qint64>("expectedValue");
332+
QTest::addColumn<bool>("isNegative");
333+
QTest::addColumn<bool>("inInt64Range");
334+
335+
// unsigned integers
336+
QTest::newRow("0") << raw("\x00") << Q_UINT64_C(0) << Q_INT64_C(0) << false << true;
337+
QTest::newRow("1") << raw("\x01") << Q_UINT64_C(1) << Q_INT64_C(1) << false << true;
338+
QTest::newRow("10") << raw("\x0a") << Q_UINT64_C(10) << Q_INT64_C(10) << false << true;
339+
QTest::newRow("23") << raw("\x17") << Q_UINT64_C(23) << Q_INT64_C(23) << false << true;
340+
QTest::newRow("24") << raw("\x18\x18") << Q_UINT64_C(24) << Q_INT64_C(24) << false << true;
341+
QTest::newRow("UINT8_MAX") << raw("\x18\xff") << Q_UINT64_C(255) << Q_INT64_C(255) << false << true;
342+
QTest::newRow("UINT8_MAX+1") << raw("\x19\x01\x00") << Q_UINT64_C(256) << Q_INT64_C(256) << false << true;
343+
QTest::newRow("UINT16_MAX") << raw("\x19\xff\xff") << Q_UINT64_C(65535) << Q_INT64_C(65535) << false << true;
344+
QTest::newRow("UINT16_MAX+1") << raw("\x1a\0\1\x00\x00") << Q_UINT64_C(65536) << Q_INT64_C(65536) << false << true;
345+
QTest::newRow("UINT32_MAX") << raw("\x1a\xff\xff\xff\xff") << Q_UINT64_C(4294967295) << Q_INT64_C(4294967295) << false << true;
346+
QTest::newRow("UINT32_MAX+1") << raw("\x1b\0\0\0\1\0\0\0\0") << Q_UINT64_C(4294967296) << Q_INT64_C(4294967296) << false << true;
347+
QTest::newRow("INT64_MAX") << raw("\x1b" "\x7f\xff\xff\xff" "\xff\xff\xff\xff")
348+
<< quint64(std::numeric_limits<qint64>::max())
349+
<< std::numeric_limits<qint64>::max() << false << true;
350+
QTest::newRow("UINT64_MAX") << raw("\x1b" "\xff\xff\xff\xff" "\xff\xff\xff\xff")
351+
<< std::numeric_limits<quint64>::max() << qint64(-123456) << false << false;
352+
353+
// negative integers
354+
QTest::newRow("-1") << raw("\x20") << Q_UINT64_C(0) << Q_INT64_C(-1) << true << true;
355+
QTest::newRow("-2") << raw("\x21") << Q_UINT64_C(1) << Q_INT64_C(-2) << true << true;
356+
QTest::newRow("-24") << raw("\x37") << Q_UINT64_C(23) << Q_INT64_C(-24) << true << true;
357+
QTest::newRow("-25") << raw("\x38\x18") << Q_UINT64_C(24) << Q_INT64_C(-25) << true << true;
358+
QTest::newRow("-UINT8_MAX") << raw("\x38\xff") << Q_UINT64_C(255) << Q_INT64_C(-256) << true << true;
359+
QTest::newRow("-UINT8_MAX-1") << raw("\x39\x01\x00") << Q_UINT64_C(256) << Q_INT64_C(-257) << true << true;
360+
QTest::newRow("-UINT16_MAX") << raw("\x39\xff\xff") << Q_UINT64_C(65535) << Q_INT64_C(-65536) << true << true;
361+
QTest::newRow("-UINT16_MAX-1") << raw("\x3a\0\1\x00\x00") << Q_UINT64_C(65536) << Q_INT64_C(-65537) << true << true;
362+
QTest::newRow("-UINT32_MAX") << raw("\x3a\xff\xff\xff\xff") << Q_UINT64_C(4294967295) << Q_INT64_C(-4294967296) << true << true;
363+
QTest::newRow("-UINT32_MAX-1") << raw("\x3b\0\0\0\1\0\0\0\0") << Q_UINT64_C(4294967296) << Q_INT64_C(-4294967297) << true << true;
364+
QTest::newRow("INT64_MIN+1") << raw("\x3b\x7f\xff\xff\xff""\xff\xff\xff\xfe")
365+
<< quint64(std::numeric_limits<qint64>::max() - 1)
366+
<< (std::numeric_limits<qint64>::min() + 1)
367+
<< true << true;
368+
QTest::newRow("INT64_MIN") << raw("\x3b\x7f\xff\xff\xff""\xff\xff\xff\xff")
369+
<< quint64(std::numeric_limits<qint64>::max())
370+
<< std::numeric_limits<qint64>::min()
371+
<< true << true;
372+
QTest::newRow("INT64_MIN-1") << raw("\x3b\x80\0\0\0""\0\0\0\0") << Q_UINT64_C(9223372036854775808) << qint64(-123456) << true << false;
373+
QTest::newRow("-UINT64_MAX") << raw("\x3b" "\xff\xff\xff\xff" "\xff\xff\xff\xfe")
374+
<< (std::numeric_limits<quint64>::max() - 1) << qint64(-123456) << true << false;
375+
QTest::newRow("-UINT64_MAX+1") << raw("\x3b" "\xff\xff\xff\xff" "\xff\xff\xff\xff")
376+
<< std::numeric_limits<quint64>::max() << qint64(-123456) << true << false;
377+
}
378+
379+
void tst_Parser::integers_data()
380+
{
381+
addIntegers();
382+
}
383+
384+
void tst_Parser::integers()
385+
{
386+
QFETCH(QByteArray, data);
387+
QFETCH(bool, isNegative);
388+
QFETCH(quint64, expectedRaw);
389+
QFETCH(qint64, expectedValue);
390+
QFETCH(bool, inInt64Range);
391+
392+
CborParser parser;
393+
CborValue first;
394+
CborError err = cbor_parser_init(reinterpret_cast<const quint8 *>(data.constData()), data.length(), 0, &parser, &first);
395+
QVERIFY2(!err, QByteArray("Got error \"") + cbor_error_string(err) + "\"");
396+
QVERIFY(cbor_value_is_integer(&first));
397+
398+
uint64_t raw;
399+
cbor_value_get_raw_integer(&first, &raw);
400+
QCOMPARE(quint64(raw), expectedRaw);
401+
402+
if (isNegative) {
403+
QVERIFY(cbor_value_is_negative_integer(&first));
404+
QVERIFY(!cbor_value_is_unsigned_integer(&first));
405+
} else {
406+
QVERIFY(!cbor_value_is_negative_integer(&first));
407+
QVERIFY(cbor_value_is_unsigned_integer(&first));
408+
}
409+
410+
int64_t value;
411+
if (inInt64Range) {
412+
cbor_value_get_int64(&first, &value);
413+
QCOMPARE(qint64(value), expectedValue);
414+
}
415+
416+
err = cbor_value_get_int64_checked(&first, &value);
417+
QCOMPARE(err, inInt64Range ? CborNoError : CborErrorDataTooLarge);
418+
419+
int ivalue;
420+
bool inIntRange = inInt64Range && (expectedValue == int(expectedValue));
421+
err = cbor_value_get_int_checked(&first, &ivalue);
422+
QCOMPARE(err, inIntRange ? CborNoError : CborErrorDataTooLarge);
423+
}
424+
325425
void tst_Parser::fixed_data()
326426
{
327427
addColumns();

0 commit comments

Comments
 (0)