From ece08710a64e09baf4bdcb71752ce20ff1d4a757 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Fri, 1 Jan 2010 17:31:14 -0800 Subject: [PATCH] Bugfixes: descriptorgen works without leaks! --- src/upb_atomic.h | 2 +- src/upb_data.c | 25 ++++++++++++++----------- src/upb_data.h | 4 ++-- src/upb_def.c | 29 +++++++++++++++++------------ tests/tests.c | 20 ++++++++++---------- tools/upbc.c | 9 ++++----- 6 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/upb_atomic.h b/src/upb_atomic.h index de2238c4c0..c2cb8bab89 100644 --- a/src/upb_atomic.h +++ b/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; diff --git a/src/upb_data.c b/src/upb_data.c index 8de0c6291d..e1aebdfc28 100644 --- a/src/upb_data.c +++ b/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); diff --git a/src/upb_data.h b/src/upb_data.h index 2a7940e042..6dc343b9cd 100644 --- a/src/upb_data.h +++ b/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); } diff --git a/src/upb_def.c b/src/upb_def.c index f8249107a8..b1c4ab289f 100644 --- a/src/upb_def.c +++ b/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. diff --git a/tests/tests.c b/tests/tests.c index 5704292e89..9479e82a2b 100644 --- a/tests/tests.c +++ b/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); diff --git a/tools/upbc.c b/tools/upbc.c index f2665f2401..ed80d395d5 100644 --- a/tools/upbc.c +++ b/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);