From 2a63e1b28397d998aca50250990c5e5eef00f561 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 1 Mar 2017 18:48:43 -0800 Subject: [PATCH 1/5] Fix a bug in slice_buffer:maybe_embiggen --- src/core/lib/slice/slice_buffer.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/core/lib/slice/slice_buffer.c b/src/core/lib/slice/slice_buffer.c index 9176dc8a420..451319c50d6 100644 --- a/src/core/lib/slice/slice_buffer.c +++ b/src/core/lib/slice/slice_buffer.c @@ -46,11 +46,6 @@ #define GROW(x) (3 * (x) / 2) static void maybe_embiggen(grpc_slice_buffer *sb) { - if (sb->base_slices != sb->slices) { - memmove(sb->base_slices, sb->slices, sb->count * sizeof(grpc_slice)); - sb->slices = sb->base_slices; - } - /* How far away from sb->base_slices is sb->slices pointer */ size_t slice_offset = (size_t)(sb->slices - sb->base_slices); size_t slice_count = sb->count + slice_offset; @@ -61,12 +56,15 @@ static void maybe_embiggen(grpc_slice_buffer *sb) { if (sb->base_slices == sb->inlined) { sb->base_slices = gpr_malloc(sb->capacity * sizeof(grpc_slice)); memcpy(sb->base_slices, sb->inlined, slice_count * sizeof(grpc_slice)); + sb->slices = sb->base_slices + slice_offset; } else { + if (sb->base_slices != sb->slices) { + memmove(sb->base_slices, sb->slices, sb->count * sizeof(grpc_slice)); + } sb->base_slices = gpr_realloc(sb->base_slices, sb->capacity * sizeof(grpc_slice)); + sb->slices = sb->base_slices; } - - sb->slices = sb->base_slices + slice_offset; } } From dbd36187824e7b7d17be0c182187c1ac5952b59f Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 21 Mar 2017 18:59:49 -0700 Subject: [PATCH 2/5] Strategy change --- src/core/lib/slice/slice_buffer.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/core/lib/slice/slice_buffer.c b/src/core/lib/slice/slice_buffer.c index 451319c50d6..c96b9c3b281 100644 --- a/src/core/lib/slice/slice_buffer.c +++ b/src/core/lib/slice/slice_buffer.c @@ -51,19 +51,23 @@ static void maybe_embiggen(grpc_slice_buffer *sb) { size_t slice_count = sb->count + slice_offset; if (slice_count == sb->capacity) { - sb->capacity = GROW(sb->capacity); - GPR_ASSERT(sb->capacity > slice_count); - if (sb->base_slices == sb->inlined) { - sb->base_slices = gpr_malloc(sb->capacity * sizeof(grpc_slice)); - memcpy(sb->base_slices, sb->inlined, slice_count * sizeof(grpc_slice)); - sb->slices = sb->base_slices + slice_offset; + if (sb->base_slices != sb->slices) { + /* Make room by moving elements if there's still space unused */ + memmove(sb->base_slices, sb->slices, sb->count * sizeof(grpc_slice)); + sb->slices = sb->base_slices; } else { - if (sb->base_slices != sb->slices) { - memmove(sb->base_slices, sb->slices, sb->count * sizeof(grpc_slice)); + /* Allocate more memory if no more space is available */ + sb->capacity = GROW(sb->capacity); + GPR_ASSERT(sb->capacity > slice_count); + if (sb->base_slices == sb->inlined) { + sb->base_slices = gpr_malloc(sb->capacity * sizeof(grpc_slice)); + memcpy(sb->base_slices, sb->inlined, slice_count * sizeof(grpc_slice)); + } else { + sb->base_slices = + gpr_realloc(sb->base_slices, sb->capacity * sizeof(grpc_slice)); } - sb->base_slices = - gpr_realloc(sb->base_slices, sb->capacity * sizeof(grpc_slice)); - sb->slices = sb->base_slices; + + sb->slices = sb->base_slices + slice_offset; } } } From 7b9cb834ff6dcc9af126c4cc541093b14cf5f691 Mon Sep 17 00:00:00 2001 From: Alena Varkockova Date: Sun, 2 Apr 2017 22:02:09 +0200 Subject: [PATCH 3/5] Print out some cli error message for getservices Currently, when you cann grpc_cli ls on an endpoint that does not have service reflection, the cli just silently ends without printing out any error or message. This PR should fix that. --- test/cpp/util/grpc_tool.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/cpp/util/grpc_tool.cc b/test/cpp/util/grpc_tool.cc index 856cd32c3ce..c7acb7c6313 100644 --- a/test/cpp/util/grpc_tool.cc +++ b/test/cpp/util/grpc_tool.cc @@ -321,6 +321,7 @@ bool GrpcTool::ListServices(int argc, const char** argv, std::vector service_list; if (!desc_db.GetServices(&service_list)) { + fprintf(stderr, "Received an error when querying services endpoint.\n"); return false; } From c6ec1155d026c91b1badb07ef1605bb747cff064 Mon Sep 17 00:00:00 2001 From: Noah Eisen Date: Wed, 5 Apr 2017 16:10:13 -0700 Subject: [PATCH 4/5] Fix error overflow bug --- src/core/lib/iomgr/error.c | 35 ++++++++++++++++-- .../clusterfuzz-testcase-6312731374256128 | Bin 0 -> 26793 bytes test/core/iomgr/error_test.c | 30 ++++++++++++++- tools/run_tests/generated/tests.json | 23 ++++++++++++ 4 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 test/core/end2end/fuzzers/server_fuzzer_corpus/clusterfuzz-testcase-6312731374256128 diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index 1dbb64e8f37..766cc8a5169 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -212,7 +212,11 @@ static uint8_t get_placement(grpc_error **err, size_t size) { GPR_ASSERT(*err); uint8_t slots = (uint8_t)(size / sizeof(intptr_t)); if ((*err)->arena_size + slots > (*err)->arena_capacity) { - (*err)->arena_capacity = (uint8_t)(3 * (*err)->arena_capacity / 2); + (*err)->arena_capacity = + (uint8_t)GPR_MIN(UINT8_MAX - 1, (3 * (*err)->arena_capacity / 2)); + if ((*err)->arena_size + slots > (*err)->arena_capacity) { + return UINT8_MAX; + } *err = gpr_realloc( *err, sizeof(grpc_error) + (*err)->arena_capacity * sizeof(intptr_t)); } @@ -223,10 +227,14 @@ static uint8_t get_placement(grpc_error **err, size_t size) { static void internal_set_int(grpc_error **err, grpc_error_ints which, intptr_t value) { - // GPR_ASSERT((*err)->ints[which] == UINT8_MAX); // TODO, enforce this uint8_t slot = (*err)->ints[which]; if (slot == UINT8_MAX) { slot = get_placement(err, sizeof(value)); + if (slot == UINT8_MAX) { + gpr_log(GPR_ERROR, "Error %p is full, dropping int {\"%s\":%" PRIiPTR "}", *err, + error_int_name(which), value); + return; + } } (*err)->ints[which] = slot; (*err)->arena[slot] = value; @@ -234,10 +242,16 @@ static void internal_set_int(grpc_error **err, grpc_error_ints which, static void internal_set_str(grpc_error **err, grpc_error_strs which, grpc_slice value) { - // GPR_ASSERT((*err)->strs[which] == UINT8_MAX); // TODO, enforce this uint8_t slot = (*err)->strs[which]; if (slot == UINT8_MAX) { slot = get_placement(err, sizeof(value)); + if (slot == UINT8_MAX) { + const char *str = grpc_slice_to_c_string(value); + gpr_log(GPR_ERROR, "Error %p is full, dropping string {\"%s\":\"%s\"}", + *err, error_str_name(which), str); + gpr_free((void *)str); + return; + } } else { unref_slice(*(grpc_slice *)((*err)->arena + slot)); } @@ -245,12 +259,19 @@ static void internal_set_str(grpc_error **err, grpc_error_strs which, memcpy((*err)->arena + slot, &value, sizeof(value)); } +static char *fmt_time(gpr_timespec tm); static void internal_set_time(grpc_error **err, grpc_error_times which, gpr_timespec value) { - // GPR_ASSERT((*err)->times[which] == UINT8_MAX); // TODO, enforce this uint8_t slot = (*err)->times[which]; if (slot == UINT8_MAX) { slot = get_placement(err, sizeof(value)); + if (slot == UINT8_MAX) { + const char *time_str = fmt_time(value); + gpr_log(GPR_ERROR, "Error %p is full, dropping \"%s\":\"%s\"}", *err, + error_time_name(which), time_str); + gpr_free((void *)time_str); + return; + } } (*err)->times[which] = slot; memcpy((*err)->arena + slot, &value, sizeof(value)); @@ -259,6 +280,12 @@ static void internal_set_time(grpc_error **err, grpc_error_times which, static void internal_add_error(grpc_error **err, grpc_error *new) { grpc_linked_error new_last = {new, UINT8_MAX}; uint8_t slot = get_placement(err, sizeof(grpc_linked_error)); + if (slot == UINT8_MAX) { + gpr_log(GPR_ERROR, "Error %p is full, dropping error %p = %s", *err, new, + grpc_error_string(new)); + GRPC_ERROR_UNREF(new); + return; + } if ((*err)->first_err == UINT8_MAX) { GPR_ASSERT((*err)->last_err == UINT8_MAX); (*err)->last_err = slot; diff --git a/test/core/end2end/fuzzers/server_fuzzer_corpus/clusterfuzz-testcase-6312731374256128 b/test/core/end2end/fuzzers/server_fuzzer_corpus/clusterfuzz-testcase-6312731374256128 new file mode 100644 index 0000000000000000000000000000000000000000..4c6eb601ae342400eaccb7d4688136d152f2b06d GIT binary patch literal 26793 zcmeI*u}Z@L6h`3)rAV>w(Ah!7r_ezt+V_8zn%nSuFE|ShUxyeYT@L>_2@dV~^ZoRA zdb?cC&#zA}$HVdP^-;%V+3%LyYyEM(u4lWu?e|=-YwM-^{dW21n7@1950cvnF80Es zc7hLl-~%7h0nZbjv)TsFpM2m0AG!m4-~%7{kmp7JUz#@bE^xu62*eZjP4~@dM0TMQ zlS=!73ob<Z4LUFgK5(!St=OA&}C`l+AOi0ncq zCYAOD7hH-!Jkd}6oJM39Ix(rVFSy`R1mcN)>gO~fyU>YArG3E#mm&~P^iw~l5!r=K zOe*aQF1Qqdc%q;BIgQ9JbYfCzUvR;t2*eZp)X!-|cA*oKO8bHfE=3@o=%;>8BeDyf zm{i&qTyQA@@kBrMa~hFd=)|PbzTkpO5r`-Hsh`t`>_R6dmG%V}T#7(E(NF!HMr0Q{ zF{!jKxZqL*;)#Ch=QJX_(1}T Date: Thu, 6 Apr 2017 15:48:19 -0700 Subject: [PATCH 5/5] clang fmt If I had a nickle... --- src/core/lib/iomgr/error.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/iomgr/error.c b/src/core/lib/iomgr/error.c index 766cc8a5169..fbbca6b4939 100644 --- a/src/core/lib/iomgr/error.c +++ b/src/core/lib/iomgr/error.c @@ -231,8 +231,8 @@ static void internal_set_int(grpc_error **err, grpc_error_ints which, if (slot == UINT8_MAX) { slot = get_placement(err, sizeof(value)); if (slot == UINT8_MAX) { - gpr_log(GPR_ERROR, "Error %p is full, dropping int {\"%s\":%" PRIiPTR "}", *err, - error_int_name(which), value); + gpr_log(GPR_ERROR, "Error %p is full, dropping int {\"%s\":%" PRIiPTR "}", + *err, error_int_name(which), value); return; } }