From cd2f77d39251be4326f9d75ba10ab8ac5686475e Mon Sep 17 00:00:00 2001
From: Joshua Haberman <joshua@reverberate.org>
Date: Fri, 28 Aug 2009 17:45:34 -0700
Subject: Incremental improvements to cbparser, and a test.

---
 Makefile        |  2 +-
 src/upb_msg.c   | 25 +++++++++++--------------
 src/upb_parse.c | 15 +++++++++++----
 src/upb_parse.h | 11 ++++++++---
 tests/tests.c   | 21 +++++++++++++++++++++
 5 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/Makefile b/Makefile
index 110a263..bcf217b 100644
--- a/Makefile
+++ b/Makefile
@@ -28,7 +28,7 @@ CC=gcc
 CXX=g++
 CFLAGS=-std=c99
 INCLUDE=-Idescriptor -Isrc -Itests -I.
-CPPFLAGS=-Wall -Wextra -g $(INCLUDE) $(strip $(shell test -f perf-cppflags && cat perf-cppflags))
+CPPFLAGS=-Wall -Wextra -Werror -g $(INCLUDE) $(strip $(shell test -f perf-cppflags && cat perf-cppflags))
 LDLIBS=-lpthread
 
 LIBUPB=src/libupb.a
diff --git a/src/upb_msg.c b/src/upb_msg.c
index 7bacc89..ce443e1 100644
--- a/src/upb_msg.c
+++ b/src/upb_msg.c
@@ -136,9 +136,6 @@ struct upb_msg_parser {
   struct upb_msg_parser_frame stack[UPB_MAX_NESTING], *top;
 };
 
-void upb_msgparser_init(struct upb_msg_parser *p,
-                        struct upb_msg *msg, bool byref);
-
 /* Parses protocol buffer data out of data which has length of len.  The data
  * need not be a complete protocol buffer.  The number of bytes parsed is
  * returned in *read, and the next call to upb_msg_parse must supply data that
@@ -255,17 +252,6 @@ static void end_cb(void *udata)
 
 /* Externally-visible functions for the msg parser. */
 
-upb_status_t upb_msg_parsestr(struct upb_msg *msg, void *buf, size_t len)
-{
-  struct upb_msg_parser mp;
-  upb_msgparser_init(&mp, msg, false);
-  size_t read;
-  upb_msg_clear(msg);
-  upb_status_t ret = upb_msg_parser_parse(&mp, buf, len, &read);
-  upb_msgparser_free(&mp);
-  return ret;
-}
-
 void upb_msgparser_init(struct upb_msg_parser *s, struct upb_msg *msg, bool byref)
 {
   s->s = upb_cbparser_new();
@@ -280,6 +266,17 @@ void upb_msgparser_free(struct upb_msg_parser *s)
   upb_cbparser_free(s->s);
 }
 
+upb_status_t upb_msg_parsestr(struct upb_msg *msg, void *buf, size_t len)
+{
+  struct upb_msg_parser mp;
+  upb_msgparser_init(&mp, msg, false);
+  size_t read;
+  upb_msg_clear(msg);
+  upb_status_t ret = upb_msg_parser_parse(&mp, buf, len, &read);
+  upb_msgparser_free(&mp);
+  return ret;
+}
+
 upb_status_t upb_msg_parser_parse(struct upb_msg_parser *s,
                                   void *data, size_t len, size_t *read)
 {
diff --git a/src/upb_parse.c b/src/upb_parse.c
index bd511e4..68df31f 100644
--- a/src/upb_parse.c
+++ b/src/upb_parse.c
@@ -158,7 +158,10 @@ static upb_status_t push(struct upb_cbparser *s, uint8_t *start,
   if(s->start_cb)
     s->start_cb(s->udata, user_field_desc);
 
-  *submsg_end = start + (*s->top > 0 ? (*s->top - s->completed_offset) : 0);
+  if(*s->top > 0)
+    *submsg_end = start + (*s->top - s->completed_offset);
+  else
+    *submsg_end = (void*)UINTPTR_MAX;
   return UPB_STATUS_OK;
 }
 
@@ -176,7 +179,7 @@ static void *pop(struct upb_cbparser *s, uint8_t *start)
   if(*s->top > 0)
     return (char*)start + (*s->top - s->completed_offset);
   else
-    return (char*)start;  // group.
+    return (void*)UINTPTR_MAX;  // group.
 }
 
 
@@ -187,7 +190,7 @@ upb_status_t upb_cbparser_parse(struct upb_cbparser *s, void *_buf, size_t len,
   uint8_t *completed = buf;
   uint8_t *const start = buf;  // ptr equivalent of s->completed_offset
   uint8_t *end = buf + len;
-  uint8_t *submsg_end = buf + (*s->top > 0 ? *s->top : 0);
+  uint8_t *submsg_end = *s->top > 0 ? buf + *s->top : (uint8_t*)UINTPTR_MAX;
   upb_status_t status = UPB_STATUS_OK;
 
   // Make local copies so optimizer knows they won't change.
@@ -238,8 +241,12 @@ upb_status_t upb_cbparser_parse(struct upb_cbparser *s, void *_buf, size_t len,
       }
     }
 
-    while(buf == submsg_end)
+    while(buf >= submsg_end) {
+      if(buf > submsg_end) {
+        return UPB_ERROR_BAD_SUBMESSAGE_END;
+      }
       submsg_end = pop(s, start);
+    }
     // while(buf < s->packed_end) { TODO: packed arrays }
     completed = buf;
   }
diff --git a/src/upb_parse.h b/src/upb_parse.h
index df26089..1dc4b4c 100644
--- a/src/upb_parse.h
+++ b/src/upb_parse.h
@@ -27,9 +27,11 @@ extern "C" {
 // client should determine whether it wants to parse or skip the corresponding
 // value.  If it wants to parse it, it must discover and return the correct
 // .proto type (the tag only contains the wire type) and check that the wire
-// type is appropriate for the .proto type.  To skip the value (which means
-// skipping all submessages, in the case of a submessage), the callback should
-// return zero.
+// type is appropriate for the .proto type.  Returning a type for which
+// upb_check_type(tag->wire_type, type) == false invokes undefined behavior.
+//
+// To skip the value (which means skipping all submessages, in the case of a
+// submessage), the callback should return zero.
 //
 // The client can store a void* in *user_field_desc; this will be passed to
 // the value callback or the string callback.
@@ -67,6 +69,9 @@ void upb_cbparser_free(struct upb_cbparser *p);
 // Resets the internal state of an already-allocated parser.  Parsers must be
 // reset before they can be used.  A parser can be reset multiple times.  udata
 // will be passed as the first argument to callbacks.
+//
+// tagcb must be set, but all other callbacks can be NULL, in which case they
+// will just be skipped.
 void upb_cbparser_reset(struct upb_cbparser *p, void *udata,
                         upb_tag_cb tagcb,
                         upb_value_cb valuecb,
diff --git a/tests/tests.c b/tests/tests.c
index e625f8a..7f6f6a4 100644
--- a/tests/tests.c
+++ b/tests/tests.c
@@ -227,6 +227,26 @@ static void test_upb_context() {
   upb_context_unref(c);
 }
 
+
+static upb_field_type_t tag_cb(void *udata, struct upb_tag *tag,
+                               void **user_field_desc)
+{
+  (void)udata;
+  (void)tag;
+  (void)user_field_desc;
+  return 0;
+}
+
+static void test_cbparser()
+{
+  struct upb_cbparser *p = upb_cbparser_new();
+  ASSERT(p);
+  upb_cbparser_reset(p, NULL, tag_cb, NULL, NULL, NULL, NULL);
+  size_t read;
+  ASSERT(upb_cbparser_parse(p, NULL, 0, &read) == UPB_STATUS_OK);
+  ASSERT(read == 0);
+}
+
 int main()
 {
 #define TEST(func) do { \
@@ -241,6 +261,7 @@ int main()
   TEST(test_skip_v_uint64_t);
   TEST(test_get_f_uint32_t);
   TEST(test_upb_context);
+  TEST(test_cbparser);
   printf("All tests passed (%d assertions).\n", num_assertions);
   return 0;
 }
-- 
cgit v1.2.3