Merge pull request #333 from haberman/32bitfixes

Fixed a critical bug on 32-bit builds, and added much more Kokoro testing

.bazelrc

@@ -0,0 +1,20 @@
+# Use our custom-configured c++ toolchain.
+
+build:m32 --copt=-m32 --linkopt=-m32
+build:asan --copt=-fsanitize=address --linkopt=-fsanitize=address
+build:valgrind --run_under='valgrind --leak-check=full --error-exitcode=1'
+
+build:ubsan --copt=-fsanitize=undefined --linkopt=-fsanitize=undefined --action_env=UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1
+# Workaround for the fact that Bazel links with $CC, not $CXX
+# https://github.com/bazelbuild/bazel/issues/11122#issuecomment-613746748
+build:ubsan --copt=-fno-sanitize=function --copt=-fno-sanitize=vptr
+
+build:Werror --copt=-Werror
+build:Werror --per_file_copt=json/parser@-Wno-error
+build:Werror --per_file_copt=com_google_protobuf@-Wno-error
+
+# GCC's -fanalyzer, a deeper static analysis than normal warnings.
+build:analyzer --copt=-fanalyzer --copt=-Werror
+build:analyzer --per_file_copt=json/parser@-fno-analyzer
+build:analyzer --per_file_copt=com_google_protobuf@-fno-analyzer
+build:analyzer --per_file_copt=com_github_google_benchmark@-fno-analyzer

bazel/build_defs.bzl

@@ -21,7 +21,12 @@ UPB_DEFAULT_COPTS = select({
         "-std=c99",
         "-pedantic",
         "-Werror=pedantic",
+        "-Wall",
         "-Wstrict-prototypes",
+        # GCC (at least) emits spurious warnings for this that cannot be fixed
+        # without introducing redundant initialization (with runtime cost):
+        #   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635
+        #"-Wno-maybe-uninitialized",
         # copybara:strip_end
     ],
 })

benchmarks/benchmark.cc

@@ -50,7 +50,7 @@ static void BM_ArenaInitialBlockOneAlloc(benchmark::State& state) {
 BENCHMARK(BM_ArenaInitialBlockOneAlloc);
 
 static void BM_LoadDescriptor_Upb(benchmark::State& state) {
-  size_t bytes_per_iter;
+  size_t bytes_per_iter = 0;
   for (auto _ : state) {
     upb::SymbolTable symtab;
     google_protobuf_DescriptorProto_getmsgdef(symtab.ptr());
@@ -61,7 +61,7 @@ static void BM_LoadDescriptor_Upb(benchmark::State& state) {
 BENCHMARK(BM_LoadDescriptor_Upb);
 
 static void BM_LoadAdsDescriptor_Upb(benchmark::State& state) {
-  size_t bytes_per_iter;
+  size_t bytes_per_iter = 0;
   for (auto _ : state) {
     upb::SymbolTable symtab;
     google_ads_googleads_v5_services_SearchGoogleAdsRequest_getmsgdef(
@@ -97,7 +97,7 @@ static void BM_LoadAdsDescriptor_Proto2(benchmark::State& state) {
   CollectFileDescriptors(
       &google_ads_googleads_v5_services_google_ads_service_proto_upbdefinit,
       serialized_files, seen_files);
-  size_t bytes_per_iter;
+  size_t bytes_per_iter = 0;
   for (auto _ : state) {
     bytes_per_iter = 0;
     protobuf::Arena arena;

kokoro/ubuntu/build.sh

@@ -11,29 +11,36 @@ fi
 echo PATH=$PATH
 ls -l `which cmake`
 cmake --version
-echo CC=${CC:-cc}
-${CC:-cc} --version
 
 # Log the bazel path and version.
 which bazel
 bazel version
 
 cd $(dirname $0)/../..
-bazel test --test_output=errors ...
 
-if [[ $(uname) = "Linux" ]]; then
-  # Verify the ASAN build.  Have to exclude test_conformance_upb as protobuf
-  # currently leaks memory in the conformance test runner.
-  bazel test --copt=-fsanitize=address --linkopt=-fsanitize=address --test_output=errors ...
+if which gcc; then
+  gcc --version
+  CC=gcc bazel test --test_output=errors ...
+  CC=gcc bazel test -c opt --test_output=errors ...
+  # TODO: work through these errors and enable this.
+  # if gcc -fanalyzer -x c /dev/null -c -o /dev/null; then
+  #   CC=gcc bazel test --copt=-fanalyzer --test_output=errors ...
+  # fi
+fi
 
-  # Verify the UBSan build. Have to exclude Lua as the version we are using
-  # fails some UBSan tests.
+if which clang; then
+  CC=clang bazel test --test_output=errors ...
+  CC=clang bazel test --test_output=errors -c opt ...
 
-  # For some reason kokoro doesn't have Clang available right now.
-  #CC=clang CXX=clang++ bazel test -c dbg --copt=-fsanitize=undefined --copt=-fno-sanitize=function,vptr --linkopt=-fsanitize=undefined --action_env=UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1 -- :all -:test_lua
+  if [[ $(uname) = "Linux" ]]; then
+    CC=clang bazel test --test_output=errors --config=m32 ...
+    CC=clang bazel test --test_output=errors --config=asan ...
 
+    # TODO: update to a newer Lua that hopefully does not trigger UBSAN.
+    CC=clang bazel test --test_output=errors --config=ubsan ... -- -tests/bindings/lua:test_lua
+  fi
 fi
 
 if which valgrind; then
-  bazel test --run_under='valgrind --leak-check=full --error-exitcode=1' ... -- -tests:test_conformance_upb -cmake:cmake_build
+  bazel test --config=valgrind ... -- -tests:test_conformance_upb -cmake:cmake_build
 fi

tests/test_generated_code.c

@@ -56,9 +56,9 @@ static void test_scalars(void) {
   ASSERT(protobuf_test_messages_proto3_TestAllTypesProto3_optional_uint64(
              msg2) == 40);
   ASSERT(protobuf_test_messages_proto3_TestAllTypesProto3_optional_float(
-             msg2) == 50.5);
+             msg2) - 50.5 < 0.01);
   ASSERT(protobuf_test_messages_proto3_TestAllTypesProto3_optional_double(
-             msg2) == 60.6);
+             msg2) - 60.6 < 0.01);
   ASSERT(protobuf_test_messages_proto3_TestAllTypesProto3_optional_bool(
              msg2) == 1);
   ASSERT(upb_strview_eql(

upb/decode.c

@@ -615,7 +615,8 @@ static const char *decode_msg(upb_decstate *d, const char *ptr, upb_msg *msg,
         int ndx = field->descriptortype;
         if (_upb_isrepeated(field)) ndx += 18;
         ptr = decode_varint32(d, ptr, &val.size);
-        if (val.size >= INT32_MAX || ptr - d->end + val.size > d->limit) {
+        if (val.size >= INT32_MAX ||
+            ptr - d->end + (int32_t)val.size > d->limit) {
           decode_err(d); /* Length overflow. */
         }
         op = delim_ops[ndx];

upb/def.c

@@ -2068,10 +2068,14 @@ static const upb_filedef *_upb_symtab_addfile(
     upb_symtab *s, const google_protobuf_FileDescriptorProto *file_proto,
     const upb_msglayout **layouts, upb_status *status) {
   upb_arena *file_arena = upb_arena_new();
-  upb_filedef *file = upb_arena_malloc(file_arena, sizeof(*file));
-  bool ok = true;
+  upb_filedef *file;
   symtab_addctx ctx;
 
+  if (!file_arena) return NULL;
+
+  file = upb_arena_malloc(file_arena, sizeof(*file));
+  if (!file) goto done;
+
   ctx.file = file;
   ctx.symtab = s;
   ctx.file_arena = file_arena;
@@ -2085,8 +2089,8 @@ static const upb_filedef *_upb_symtab_addfile(
 
   if (UPB_UNLIKELY(setjmp(ctx.err))) {
     UPB_ASSERT(!upb_ok(status));
-    ok = false;
     remove_filedef(s, file);
+    file = NULL;
   } else {
     build_filedef(&ctx, file, file_proto);
     upb_strtable_insert3(&s->files, file->name, strlen(file->name),
@@ -2095,8 +2099,9 @@ static const upb_filedef *_upb_symtab_addfile(
     upb_arena_fuse(s->arena, file_arena);
   }
 
+done:
   upb_arena_free(file_arena);
-  return ok ? file : NULL;
+  return file;
 }
 
 const upb_filedef *upb_symtab_addfile(

upb/json_decode.c

@@ -396,6 +396,8 @@ static void jsondec_resize(jsondec *d, char **buf, char **end, char **buf_end) {
   size_t size = UPB_MAX(8, 2 * oldsize);
 
   *buf = upb_arena_realloc(d->arena, *buf, len, size);
+  if (!*buf) jsondec_err(d, "Out of memory");
+
   *end = *buf + len;
   *buf_end = *buf + size;
 }

upb/msg.c

@@ -11,7 +11,7 @@ static const size_t overhead = sizeof(upb_msg_internal);
 
 static const upb_msg_internal *upb_msg_getinternal_const(const upb_msg *msg) {
   ptrdiff_t size = sizeof(upb_msg_internal);
-  return UPB_PTR_AT(msg, -size, upb_msg_internal);
+  return (upb_msg_internal*)((char*)msg - size);
 }
 
 upb_msg *_upb_msg_new(const upb_msglayout *l, upb_arena *a) {
@@ -111,13 +111,16 @@ void *_upb_array_resize_fallback(upb_array **arr_ptr, size_t size,
 bool _upb_array_append_fallback(upb_array **arr_ptr, const void *value,
                                 int elem_size_lg2, upb_arena *arena) {
   upb_array *arr = getorcreate_array(arr_ptr, elem_size_lg2, arena);
-  size_t elem = arr->len;
-  char *data;
+  if (!arr) return false;
 
-  if (!arr || !_upb_array_resize(arr, elem + 1, arena)) return false;
+  size_t elems = arr->len;
 
-  data = _upb_array_ptr(arr);
-  memcpy(data + (elem << elem_size_lg2), value, 1 << elem_size_lg2);
+  if (!_upb_array_resize(arr, elems + 1, arena)) {
+    return false;
+  }
+
+  char *data = _upb_array_ptr(arr);
+  memcpy(data + (elems << elem_size_lg2), value, 1 << elem_size_lg2);
   return true;
 }
 

upb/msg.h

@@ -94,7 +94,8 @@ UPB_INLINE upb_msg *_upb_msg_new_inl(const upb_msglayout *l, upb_arena *a) {
 upb_msg *_upb_msg_new(const upb_msglayout *l, upb_arena *a);
 
 UPB_INLINE upb_msg_internal *upb_msg_getinternal(upb_msg *msg) {
-  return UPB_PTR_AT(msg, -sizeof(upb_msg_internal), upb_msg_internal);
+  ptrdiff_t size = sizeof(upb_msg_internal);
+  return (upb_msg_internal*)((char*)msg - size);
 }
 
 /* Clears the given message. */
@@ -189,9 +190,11 @@ typedef struct {
   uintptr_t data;   /* Tagged ptr: low 3 bits of ptr are lg2(elem size). */
   size_t len;   /* Measured in elements. */
   size_t size;  /* Measured in elements. */
+  uint64_t junk;
 } upb_array;
 
 UPB_INLINE const void *_upb_array_constptr(const upb_array *arr) {
+  UPB_ASSERT((arr->data & 7) <= 4);
   return (void*)(arr->data & ~(uintptr_t)7);
 }
 
@@ -201,15 +204,17 @@ UPB_INLINE void *_upb_array_ptr(upb_array *arr) {
 
 UPB_INLINE uintptr_t _upb_tag_arrptr(void* ptr, int elem_size_lg2) {
   UPB_ASSERT(elem_size_lg2 <= 4);
+  UPB_ASSERT(((uintptr_t)ptr & 7) == 0);
   return (uintptr_t)ptr | (unsigned)elem_size_lg2;
 }
 
 UPB_INLINE upb_array *_upb_array_new(upb_arena *a, size_t init_size,
                                      int elem_size_lg2) {
+  const size_t arr_size = UPB_ALIGN_UP(sizeof(upb_array), 8);
   const size_t bytes = sizeof(upb_array) + (init_size << elem_size_lg2);
   upb_array *arr = (upb_array*)upb_arena_malloc(a, bytes);
   if (!arr) return NULL;
-  arr->data = _upb_tag_arrptr(arr + 1, elem_size_lg2);
+  arr->data = _upb_tag_arrptr(UPB_PTR_AT(arr, arr_size, void), elem_size_lg2);
   arr->len = 0;
   arr->size = init_size;
   return arr;
@@ -382,17 +387,17 @@ UPB_INLINE void _upb_map_fromkey(upb_strview key, void* out, size_t size) {
   }
 }
 
-UPB_INLINE upb_value _upb_map_tovalue(const void *val, size_t size,
-                                      upb_arena *a) {
-  upb_value ret = {0};
+UPB_INLINE bool _upb_map_tovalue(const void *val, size_t size, upb_value *msgval,
+                                 upb_arena *a) {
   if (size == UPB_MAPTYPE_STRING) {
     upb_strview *strp = (upb_strview*)upb_arena_malloc(a, sizeof(*strp));
+    if (!strp) return false;
     *strp = *(upb_strview*)val;
-    ret = upb_value_ptr(strp);
+    *msgval = upb_value_ptr(strp);
   } else {
-    memcpy(&ret, val, size);
+    memcpy(msgval, val, size);
   }
-  return ret;
+  return true;
 }
 
 UPB_INLINE void _upb_map_fromvalue(upb_value val, void* out, size_t size) {
@@ -434,7 +439,8 @@ UPB_INLINE void* _upb_map_next(const upb_map *map, size_t *iter) {
 UPB_INLINE bool _upb_map_set(upb_map *map, const void *key, size_t key_size,
                              void *val, size_t val_size, upb_arena *arena) {
   upb_strview strkey = _upb_map_tokey(key, key_size);
-  upb_value tabval = _upb_map_tovalue(val, val_size, arena);
+  upb_value tabval = {0};
+  if (!_upb_map_tovalue(val, val_size, &tabval, arena)) return false;
   upb_alloc *a = upb_arena_alloc(arena);
 
   /* TODO(haberman): add overwrite operation to minimize number of lookups. */

upb/upb.h

@@ -314,7 +314,7 @@ UPB_INLINE uint64_t _upb_be_swap64(uint64_t val) {
 }
 
 UPB_INLINE int _upb_lg2ceil(int x) {
-  if (x == 0) return 0;
+  if (x <= 1) return 0;
 #ifdef __GNUC__
   return 32 - __builtin_clz(x - 1);
 #else