Refactored main decode loop to remove check against bytes_left

This is part of the upcoming fast decode commit. The fast path didn"t compare against bytes_left in the main loop, which caused a number of tests to fail as unexpected end-of-stream errors were raised on the stream. This change hoists the check for bytes_left from pb_decode_varint32_eof, up into pb_decode_tag, such that eof is set before an error is raised when called with bytes_left == 0. This simplifies the main loop from - while (stream->bytes_left) if (!pb_decode_tag()) ... to - while (pb_decode_tag()) ... This is also important because bytes_left isn"t guaranteed to be valid for callback streams. This created an edge case where callback streams would set an "io error" when eof was hit, regardless of if decoding was successful or not. This error could then mask actual errors generated after the main loop (e.g. missing required fields). A workaround and accompanying regression test was added for this, and is being further tracked in issue nanopb#1017.
inolen · Nov 15, 2024 · ace67f6 · ace67f6
1 parent 092e7d7
commit ace67f6
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 18 deletions.
diff --git a/pb_decode.c b/pb_decode.c
@@ -23,7 +23,6 @@
  **************************************/
 
 static bool checkreturn buf_read(pb_istream_t *stream, pb_byte_t *buf, size_t count);
-static bool checkreturn pb_decode_varint32_eof(pb_istream_t *stream, uint32_t *dest, bool *eof);
 static bool checkreturn read_raw_value(pb_istream_t *stream, pb_wire_type_t wire_type, pb_byte_t *buf, size_t *size);
 static bool checkreturn decode_basic_field(pb_istream_t *stream, pb_wire_type_t wire_type, pb_field_iter_t *field);
 static bool checkreturn decode_static_field(pb_istream_t *stream, pb_wire_type_t wire_type, pb_field_iter_t *field);
@@ -164,25 +163,18 @@ pb_istream_t pb_istream_from_buffer(const pb_byte_t *buf, size_t msglen)
     return stream;
 }
 
+
 /********************
  * Helper functions *
  ********************/
 
-static bool checkreturn pb_decode_varint32_eof(pb_istream_t *stream, uint32_t *dest, bool *eof)
+bool checkreturn pb_decode_varint32(pb_istream_t *stream, uint32_t *dest)
 {
     pb_byte_t byte;
     uint32_t result;
 
     if (!pb_readbyte(stream, &byte))
     {
-        if (stream->bytes_left == 0)
-        {
-            if (eof)
-            {
-                *eof = true;
-            }
-        }
-
         return false;
     }
 
@@ -234,11 +226,6 @@ static bool checkreturn pb_decode_varint32_eof(pb_istream_t *stream, uint32_t *d
    return true;
 }
 
-bool checkreturn pb_decode_varint32(pb_istream_t *stream, uint32_t *dest)
-{
-    return pb_decode_varint32_eof(stream, dest, NULL);
-}
-
 #ifndef PB_WITHOUT_64BIT
 bool checkreturn pb_decode_varint(pb_istream_t *stream, uint64_t *dest)
 {
@@ -294,9 +281,32 @@ bool checkreturn pb_decode_tag(pb_istream_t *stream, pb_wire_type_t *wire_type,
     *eof = false;
     *wire_type = (pb_wire_type_t) 0;
     *tag = 0;
-    
-    if (!pb_decode_varint32_eof(stream, &temp, eof))
+
+    if (stream->bytes_left == 0)
     {
+        *eof = true;
+        return false;
+    }
+
+    if (!pb_decode_varint32(stream, &temp))
+    {
+#ifndef PB_BUFFER_ONLY
+        /* Workaround for issue #1017
+         *
+         * Callback streams don't set bytes_left to 0 on eof until after being called by pb_decode_varint32,
+         * which results in "io error" being raised. This contrasts the behavior of buffer streams who raise
+         * no error on eof as bytes_left is already 0 on entry. This causes legitimate errors (e.g. missing
+         * required fields) to be incorrectly reported by callback streams.
+         */
+        if (stream->callback != buf_read && stream->bytes_left == 0)
+        {
+#ifndef PB_NO_ERRMSG
+            if (!strcmp(stream->errmsg, "io error"))
+                stream->errmsg = NULL;
+#endif
+            *eof = true;
+        }
+#endif
         return false;
     }
 
@@ -1024,7 +1034,7 @@ static bool checkreturn pb_decode_inner(pb_istream_t *stream, const pb_msgdesc_t
         }
     }
 
-    while (stream->bytes_left)
+    while (1)
     {
         uint32_t tag;
         pb_wire_type_t wire_type;

diff --git a/tests/regression/issue_1017/SConscript b/tests/regression/issue_1017/SConscript
@@ -0,0 +1,13 @@
+# Regression test for #1017:
+# Test that missing required fields are correctly reported by all streams.
+
+Import("env")
+
+env.NanopbProto("test.proto")
+
+p = env.Program(["test.c",
+                 "test.pb.c",
+                 "$COMMON/pb_decode.o",
+                 "$COMMON/pb_common.o"])
+
+env.RunTest(p)
diff --git a/tests/regression/issue_1017/test.c b/tests/regression/issue_1017/test.c
@@ -0,0 +1,54 @@
+#include <assert.h>
+#include <pb_decode.h>
+#include "unittests.h"
+#include "test.pb.h"
+#include <stdio.h>
+
+const uint8_t input_data[] = {
+    0x08, 0x01,
+};
+
+const size_t input_len = sizeof(input_data);
+
+bool stream_callback(pb_istream_t *stream, uint8_t *buf, size_t count)
+{
+    size_t cursor = (size_t)(uintptr_t)stream->state;
+
+    if ((cursor + count) > input_len)
+    {
+        stream->bytes_left = 0;
+        return false;
+    }
+
+    memcpy(buf, &input_data[cursor], count);
+    cursor += count;
+
+    *(uintptr_t *)(&stream->state) = (uintptr_t)cursor;
+
+    return true;
+}
+
+int main()
+{
+    int status = 0;
+
+    /* test buffer stream */
+    {
+        TestMessage msg = TestMessage_init_zero;
+        pb_istream_t stream = pb_istream_from_buffer(input_data, input_len);
+        TEST(pb_decode(&stream, TestMessage_fields, &msg));
+        TEST(msg.foo == 0x1);
+        TEST(stream.errmsg == NULL);
+    }
+
+    /* test callback stream */
+    {
+        TestMessage msg = TestMessage_init_zero;
+        pb_istream_t stream = {&stream_callback, 0, SIZE_MAX};
+        TEST(pb_decode(&stream, TestMessage_fields, &msg));
+        TEST(msg.foo == 0x1);
+        TEST(stream.errmsg == NULL);
+    }
+
+    return status;
+}
diff --git a/tests/regression/issue_1017/test.proto b/tests/regression/issue_1017/test.proto
@@ -0,0 +1,6 @@
+syntax = "proto2";
+
+message TestMessage {
+    required int32 foo = 1;
+    optional int32 bar = 2;
+}