Bugfixes: descriptorgen works without leaks!

src/upb_atomic.h

@@ -59,7 +59,7 @@ INLINE bool upb_atomic_add(upb_atomic_refcount_t *a, int val) {
   return a->v == 0;
 }
 
-INLINE bool upb_atomic_fetch_and_add(upb_atomic_refcount_t *a, int val) {
+INLINE int upb_atomic_fetch_and_add(upb_atomic_refcount_t *a, int val) {
   int ret = a->v;
   a->v += val;
   return ret;

src/upb_data.c

@@ -24,19 +24,21 @@ static uint32_t round_up_to_pow2(uint32_t v)
 
 /* upb_data *******************************************************************/
 
-static void data_elem_unref(void *d, struct upb_fielddef *f) {
-  if(f->type == UPB_TYPE(MESSAGE) || f->type == UPB_TYPE(GROUP)) {
-    upb_msg_unref((upb_msg*)d, upb_downcast_msgdef(f->def));
-  } else if(f->type == UPB_TYPE(STRING) || f->type == UPB_TYPE(BYTES)) {
-    upb_string_unref((upb_string*)d);
+static void data_elem_unref(union upb_value_ptr p, struct upb_fielddef *f) {
+  if(upb_issubmsg(f)) {
+    upb_msg_unref(*p.msg, upb_downcast_msgdef(f->def));
+  } else if(upb_isstring(f)) {
+    upb_string_unref(*p.str);
+  } else {
+    assert(false);
   }
 }
 
-static void data_unref(void *d, struct upb_fielddef *f) {
+static void data_unref(union upb_value_ptr p, struct upb_fielddef *f) {
   if(upb_isarray(f)) {
-    upb_array_unref((upb_array*)d, f);
+    upb_array_unref(*p.arr, f);
   } else {
-    data_elem_unref(d, f);
+    data_elem_unref(p, f);
   }
 }
 
@@ -179,7 +181,8 @@ void _upb_array_free(upb_array *a, struct upb_fielddef *f)
   if(upb_elem_ismm(f)) {
     for(upb_arraylen_t i = 0; i < a->refcounted.size; i++) {
       union upb_value_ptr p = _upb_array_getptr(a, f, i);
-      data_elem_unref(p._void, f);
+      if(!*p.data) continue;
+      data_elem_unref(p, f);
     }
   }
   if(a->refcounted.size != 0) free(a->common.elements._void);
@@ -241,8 +244,8 @@ void _upb_msg_free(upb_msg *msg, struct upb_msgdef *md)
   for(int i = 0; i < md->num_fields; i++) {
     struct upb_fielddef *f = &md->fields[i];
     union upb_value_ptr p = _upb_msg_getptr(msg, f);
-    if(!upb_field_ismm(f) || !p._void) continue;
-    data_unref(p._void, f);
+    if(!upb_field_ismm(f) || !*p.data) continue;
+    data_unref(p, f);
   }
   upb_def_unref(UPB_UPCAST(md));
   free(msg);

src/upb_data.h

@@ -374,14 +374,14 @@ void _upb_array_free(upb_array *a, struct upb_fielddef *f);
 INLINE union upb_value_ptr _upb_array_getptr(upb_array *a,
                                              struct upb_fielddef *f,
                                              upb_arraylen_t elem) {
-  assert(elem < upb_array_len(a));
   size_t type_size = upb_type_info[f->type].size;
   union upb_value_ptr p = {._void = &a->common.elements.uint8[elem * type_size]};
   return p;
 }
 
 INLINE union upb_value upb_array_get(upb_array *a, struct upb_fielddef *f,
-                                     int elem) {
+                                     upb_arraylen_t elem) {
+  assert(elem < upb_array_len(a));
   return upb_value_read(_upb_array_getptr(a, f, elem), f->type);
 }
 

src/upb_def.c

@@ -216,6 +216,11 @@ static void fielddef_uninit(struct upb_fielddef *f)
   }
 }
 
+static void fielddef_free(struct upb_fielddef *f) {
+  fielddef_uninit(f);
+  free(f);
+}
+
 static void fielddef_copy(struct upb_fielddef *dst, struct upb_fielddef *src)
 {
   *dst = *src;
@@ -501,9 +506,10 @@ static void insert_enum(struct upb_strtable *t,
   if(!fqname) return;
 
   struct symtab_ent e;
-  e.e.key = fqname;  // Donating our ref to the table.
+  e.e.key = fqname;
   e.def = UPB_UPCAST(enumdef_new(ed, fqname));
   upb_strtable_insert(t, &e.e);
+  upb_string_unref(fqname);
 }
 
 static void insert_message(struct upb_strtable *t,
@@ -518,17 +524,20 @@ static void insert_message(struct upb_strtable *t,
   int num_fields = d->set_flags.has.field ? d->field->len : 0;
   struct symtab_ent e;
   e.e.key = fqname;
+
+  // Gather our list of fields, sorting if necessary.
   struct upb_fielddef **fielddefs = malloc(sizeof(*fielddefs) * num_fields);
   for (int i = 0; i < num_fields; i++) {
     google_protobuf_FieldDescriptorProto *fd = d->field->elements[i];
     fielddefs[i] = fielddef_new(fd);
   }
   if(sort) fielddef_sort(fielddefs, d->field->len);
+
+  // Create the msgdef with that list of fields.
   e.def = UPB_UPCAST(msgdef_new(fielddefs, d->field->len, fqname, status));
-  for (int i = 0; i < num_fields; i++) {
-    fielddef_uninit(fielddefs[i]);
-    free(fielddefs[i]);
-  }
+
+  // Cleanup.
+  for (int i = 0; i < num_fields; i++) fielddef_free(fielddefs[i]);
   free(fielddefs);
 
   if(!upb_ok(status)) goto error;
@@ -545,6 +554,7 @@ static void insert_message(struct upb_strtable *t,
       insert_enum(t, d->enum_type->elements[i], fqname, status);
 
 error:
+  // Free the ref we got from try_define().
   upb_string_unref(fqname);
 }
 
@@ -601,15 +611,10 @@ static void addfd(struct upb_strtable *addto, struct upb_strtable *existingdefs,
                   struct upb_status *status)
 {
   upb_string *pkg;
-  // Temporary hack until the static data is integrated into our
-  // memory-management scheme.
-  bool should_unref;
   if(fd->set_flags.has.package) {
-    pkg = fd->package;
-    should_unref = false;
+    pkg = upb_string_getref(fd->package, UPB_REF_FROZEN);
   } else {
     pkg = upb_string_new();
-    should_unref = true;
   }
 
   if(fd->set_flags.has.message_type)
@@ -620,7 +625,7 @@ static void addfd(struct upb_strtable *addto, struct upb_strtable *existingdefs,
     for(unsigned int i = 0; i < fd->enum_type->len; i++)
       insert_enum(addto, fd->enum_type->elements[i], pkg, status);
 
-  if(should_unref) upb_string_unref(pkg);
+  upb_string_unref(pkg);
 
   if(!upb_ok(status)) {
     // TODO: make sure we don't leak any memory in this case.

tests/tests.c

@@ -16,8 +16,8 @@ static void test_get_v_uint64_t()
 {
 #define TEST(name, bytes, val) {\
     struct upb_status status = UPB_STATUS_INIT; \
-    uint8_t name[] = bytes; \
-    uint8_t *name ## _buf = name; \
+    const uint8_t name[] = bytes; \
+    const uint8_t *name ## _buf = name; \
     uint64_t name ## _val = 0; \
     name ## _buf = upb_get_v_uint64_t(name, name + sizeof(name) - 1, &name ## _val, &status); \
     ASSERT(upb_ok(&status)); \
@@ -80,8 +80,8 @@ static void test_get_v_uint32_t()
 {
 #define TEST(name, bytes, val) {\
     struct upb_status status = UPB_STATUS_INIT; \
-    uint8_t name[] = bytes; \
-    uint8_t *name ## _buf = name; \
+    const uint8_t name[] = bytes; \
+    const uint8_t *name ## _buf = name; \
     uint32_t name ## _val = 0; \
     name ## _buf = upb_get_v_uint32_t(name, name + sizeof(name), &name ## _val, &status); \
     ASSERT(upb_ok(&status)); \
@@ -145,8 +145,8 @@ static void test_skip_v_uint64_t()
 {
 #define TEST(name, bytes) {\
     struct upb_status status = UPB_STATUS_INIT; \
-    uint8_t name[] = bytes; \
-    uint8_t *name ## _buf = name; \
+    const uint8_t name[] = bytes; \
+    const uint8_t *name ## _buf = name; \
     name ## _buf = upb_skip_v_uint64_t(name ## _buf, name + sizeof(name), &status); \
     ASSERT(upb_ok(&status)); \
     ASSERT(name ## _buf == name + sizeof(name) - 1);  /* - 1 for NULL */ \
@@ -206,8 +206,8 @@ static void test_get_f_uint32_t()
 {
 #define TEST(name, bytes, val) {\
     struct upb_status status = UPB_STATUS_INIT; \
-    uint8_t name[] = bytes; \
-    uint8_t *name ## _buf = name; \
+    const uint8_t name[] = bytes; \
+    const uint8_t *name ## _buf = name; \
     uint32_t name ## _val = 0; \
     name ## _buf = upb_get_f_uint32_t(name ## _buf, name + sizeof(name), &name ## _val, &status); \
     ASSERT(upb_ok(&status)); \
@@ -230,7 +230,7 @@ static void test_get_f_uint32_t()
 static void test_upb_symtab() {
   struct upb_symtab *s = upb_symtab_new();
   ASSERT(s);
-  struct upb_string *descriptor = upb_strreadfile("tests/test.proto.pb");
+  upb_string *descriptor = upb_strreadfile("tests/test.proto.pb");
   if(!descriptor) {
     fprintf(stderr, "Couldn't read input file tests/test.proto.pb\n");
     exit(1);
@@ -242,7 +242,7 @@ static void test_upb_symtab() {
 
   // Test cycle detection by making a cyclic def's main refcount go to zero
   // and then be incremented to one again.
-  struct upb_string *symname = upb_strdupc("A");
+  upb_string *symname = upb_strdupc("A");
   struct upb_def *def = upb_symtab_lookup(s, symname);
   upb_string_unref(symname);
   ASSERT(def);

tools/upbc.c

@@ -320,20 +320,20 @@ struct typetable_entry *get_or_insert_typeentry(struct upb_strtable *t,
                                             upb_strdupc(upb_type_info[f->type].ctype);
   struct typetable_entry *type_e = upb_strtable_lookup(t, type_name);
   if(type_e == NULL) {
+    upb_string *cident = upb_strdup(type_name);
+    to_cident(cident);
     struct typetable_entry new_type_e = {
-      .e = {.key = type_name}, .field = f, .cident = upb_strdup(type_name),
+      .e = {.key = type_name}, .field = f, .cident = cident,
       .values = NULL, .values_size = 0, .values_len = 0,
       .arrays = NULL, .arrays_size = 0, .arrays_len = 0
     };
-    to_cident(new_type_e.cident);
     assert(upb_strtable_lookup(t, type_name) == NULL);
     assert(upb_strtable_lookup(t, new_type_e.e.key) == NULL);
     upb_strtable_insert(t, &new_type_e.e);
     type_e = upb_strtable_lookup(t, type_name);
     assert(type_e);
-  } else {
-    upb_string_unref(type_name);
   }
+  upb_string_unref(type_name);
   return type_e;
 }
 
@@ -592,7 +592,6 @@ static void write_message_c(upb_msg *msg, struct upb_msgdef *md,
 
   /* Free tables. */
   for(e = upb_strtable_begin(&types); e; e = upb_strtable_next(&types, &e->e)) {
-    upb_string_unref(e->e.key);
     upb_string_unref(e->cident);
     free(e->values);
     free(e->arrays);