Detect too large varint values when decoding.

Because Issue #139 now allows limiting integer fields, it is good
to check the values received from other protobuf libraries against
the lower limits.

3 files changed, 53 insertions, 17 deletions

diff --git a/pb_decode.c b/pb_decode.c
index b5ec1ef0..5982c8e5 100644
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -1072,53 +1072,75 @@ bool pb_decode_fixed64(pb_istream_t *stream, void *dest)
 static bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
     uint64_t value;
+    int64_t svalue;
+    int64_t clamped;
     if (!pb_decode_varint(stream, &value))
         return false;
     
+    /* See issue 97: Google's C++ protobuf allows negative varint values to
+     * be cast as int32_t, instead of the int64_t that should be used when
+     * encoding. Previous nanopb versions had a bug in encoding. In order to
+     * not break decoding of such messages, we cast <=32 bit fields to
+     * int32_t first to get the sign correct.
+     */
+    if (field->data_size == 8)
+        svalue = (int64_t)value;
+    else
+        svalue = (int32_t)value;
+
     switch (field->data_size)
     {
-        case 1: *(int8_t*)dest = (int8_t)value; break;
-        case 2: *(int16_t*)dest = (int16_t)value; break;
-        case 4: *(int32_t*)dest = (int32_t)value; break;
-        case 8: *(int64_t*)dest = (int64_t)value; break;
+        case 1: clamped = *(int8_t*)dest = (int8_t)svalue; break;
+        case 2: clamped = *(int16_t*)dest = (int16_t)svalue; break;
+        case 4: clamped = *(int32_t*)dest = (int32_t)svalue; break;
+        case 8: clamped = *(int64_t*)dest = svalue; break;
         default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
+
+    if (clamped != svalue)
+        PB_RETURN_ERROR(stream, "integer too large");
     
     return true;
 }
 
 static bool checkreturn pb_dec_uvarint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
-    uint64_t value;
+    uint64_t value, clamped;
     if (!pb_decode_varint(stream, &value))
         return false;
     
     switch (field->data_size)
     {
-        case 1: *(uint8_t*)dest = (uint8_t)value; break;
-        case 2: *(uint16_t*)dest = (uint16_t)value; break;
-        case 4: *(uint32_t*)dest = (uint32_t)value; break;
-        case 8: *(uint64_t*)dest = value; break;
+        case 1: clamped = *(uint8_t*)dest = (uint8_t)value; break;
+        case 2: clamped = *(uint16_t*)dest = (uint16_t)value; break;
+        case 4: clamped = *(uint32_t*)dest = (uint32_t)value; break;
+        case 8: clamped = *(uint64_t*)dest = value; break;
         default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
     
+    if (clamped != value)
+        PB_RETURN_ERROR(stream, "integer too large");
+
     return true;
 }
 
 static bool checkreturn pb_dec_svarint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
-    int64_t value;
+    int64_t value, clamped;
     if (!pb_decode_svarint(stream, &value))
         return false;
     
     switch (field->data_size)
     {
-        case 1: *(int8_t*)dest = (int8_t)value; break;
-        case 2: *(int16_t*)dest = (int16_t)value; break;
-        case 4: *(int32_t*)dest = (int32_t)value; break;
-        case 8: *(int64_t*)dest = value; break;
+        case 1: clamped = *(int8_t*)dest = (int8_t)value; break;
+        case 2: clamped = *(int16_t*)dest = (int16_t)value; break;
+        case 4: clamped = *(int32_t*)dest = (int32_t)value; break;
+        case 8: clamped = *(int64_t*)dest = value; break;
         default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
+
+    if (clamped != value)
+        PB_RETURN_ERROR(stream, "integer too large");
     
     return true;
 }
diff --git a/tests/decode_unittests/decode_unittests.c b/tests/decode_unittests/decode_unittests.c
index 8c12f1cd..47f0fbdb 100644
--- a/tests/decode_unittests/decode_unittests.c
+++ b/tests/decode_unittests/decode_unittests.c
@@ -123,16 +123,16 @@ int main()
     }
     
     {
-        pb_istream_t s = S("\x01\xFF\xFF\x03");
+        pb_istream_t s = S("\x01\x00");
         pb_field_t f = {1, PB_LTYPE_VARINT, 0, 0, 4, 0, 0};
         uint32_t d;
         COMMENT("Test pb_dec_varint using uint32_t")
         TEST(pb_dec_varint(&s, &f, &d) && d == 1)
         
         /* Verify that no more than data_size is written. */
-        d = 0;
+        d = 0xFFFFFFFF;
         f.data_size = 1;
-        TEST(pb_dec_varint(&s, &f, &d) && (d == 0xFF || d == 0xFF000000))
+        TEST(pb_dec_varint(&s, &f, &d) && (d == 0xFFFFFF00 || d == 0x00FFFFFF))
     }
     
     {
diff --git a/tests/intsizes/intsizes_unittests.c b/tests/intsizes/intsizes_unittests.c
index 29cc7ab0..189825f7 100644
--- a/tests/intsizes/intsizes_unittests.c
+++ b/tests/intsizes/intsizes_unittests.c
@@ -101,6 +101,20 @@ int main()
                    INT32_MIN, 0, INT32_MIN,
                    INT64_MIN, 0, INT64_MIN, true);
 
+    COMMENT("Test overflow detection");
+    TEST_ROUNDTRIP(-129,      0,   -128,
+                   -32768,    0, -32768,
+                   INT32_MIN, 0, INT32_MIN,
+                   INT64_MIN, 0, INT64_MIN, false);
+    TEST_ROUNDTRIP(127,     256,    127,
+                   32767, 65535,  32767,
+                   INT32_MAX, UINT32_MAX, INT32_MAX,
+                   INT64_MAX, UINT64_MAX, INT64_MAX, false);
+    TEST_ROUNDTRIP(-128,      0,   -128,
+                   -32768,    0, -32769,
+                   INT32_MIN, 0, INT32_MIN,
+                   INT64_MIN, 0, INT64_MIN, false);
+    
     if (status != 0)
         fprintf(stdout, "\n\nSome tests FAILED!\n");