From f8a4aae1197e586d5c34f1acff450eac4d764c4b Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 1 Aug 2018 08:36:03 -0700 Subject: Fix all instances of bugprone-undefined-memory-manipulation --- test/core/end2end/fixtures/http_proxy_fixture.cc | 18 ++++++++++++------ test/core/end2end/fixtures/proxy.cc | 17 +++++++++++++---- 2 files changed, 25 insertions(+), 10 deletions(-) (limited to 'test/core/end2end') diff --git a/test/core/end2end/fixtures/http_proxy_fixture.cc b/test/core/end2end/fixtures/http_proxy_fixture.cc index f02fa9d998..6f444bf66a 100644 --- a/test/core/end2end/fixtures/http_proxy_fixture.cc +++ b/test/core/end2end/fixtures/http_proxy_fixture.cc @@ -52,6 +52,16 @@ #include "test/core/util/port.h" struct grpc_end2end_http_proxy { + grpc_end2end_http_proxy() + : proxy_name(nullptr), + server(nullptr), + channel_args(nullptr), + mu(nullptr), + pollset(nullptr), + combiner(nullptr) { + gpr_ref_init(&users, 1); + combiner = grpc_combiner_create(); + } char* proxy_name; grpc_core::Thread thd; grpc_tcp_server* server; @@ -519,11 +529,7 @@ static void thread_main(void* arg) { grpc_end2end_http_proxy* grpc_end2end_http_proxy_create( grpc_channel_args* args) { grpc_core::ExecCtx exec_ctx; - grpc_end2end_http_proxy* proxy = - static_cast(gpr_malloc(sizeof(*proxy))); - memset(proxy, 0, sizeof(*proxy)); - proxy->combiner = grpc_combiner_create(); - gpr_ref_init(&proxy->users, 1); + grpc_end2end_http_proxy* proxy = grpc_core::New(); // Construct proxy address. const int proxy_port = grpc_pick_unused_port_or_die(); gpr_join_host_port(&proxy->proxy_name, "localhost", proxy_port); @@ -573,7 +579,7 @@ void grpc_end2end_http_proxy_destroy(grpc_end2end_http_proxy* proxy) { GRPC_CLOSURE_CREATE(destroy_pollset, proxy->pollset, grpc_schedule_on_exec_ctx)); GRPC_COMBINER_UNREF(proxy->combiner, "test"); - gpr_free(proxy); + grpc_core::Delete(proxy); } const char* grpc_end2end_http_proxy_get_proxy_name( diff --git a/test/core/end2end/fixtures/proxy.cc b/test/core/end2end/fixtures/proxy.cc index 042c858b4c..869b6e846d 100644 --- a/test/core/end2end/fixtures/proxy.cc +++ b/test/core/end2end/fixtures/proxy.cc @@ -30,6 +30,17 @@ #include "test/core/util/port.h" struct grpc_end2end_proxy { + grpc_end2end_proxy() + : proxy_port(nullptr), + server_port(nullptr), + cq(nullptr), + server(nullptr), + client(nullptr), + shutdown(false), + new_call(nullptr) { + memset(&new_call_details, 0, sizeof(new_call_details)); + memset(&new_call_metadata, 0, sizeof(new_call_metadata)); + } grpc_core::Thread thd; char* proxy_port; char* server_port; @@ -79,9 +90,7 @@ grpc_end2end_proxy* grpc_end2end_proxy_create(const grpc_end2end_proxy_def* def, int proxy_port = grpc_pick_unused_port_or_die(); int server_port = grpc_pick_unused_port_or_die(); - grpc_end2end_proxy* proxy = - static_cast(gpr_malloc(sizeof(*proxy))); - memset(proxy, 0, sizeof(*proxy)); + grpc_end2end_proxy* proxy = grpc_core::New(); gpr_join_host_port(&proxy->proxy_port, "localhost", proxy_port); gpr_join_host_port(&proxy->server_port, "localhost", server_port); @@ -128,7 +137,7 @@ void grpc_end2end_proxy_destroy(grpc_end2end_proxy* proxy) { grpc_channel_destroy(proxy->client); grpc_completion_queue_destroy(proxy->cq); grpc_call_details_destroy(&proxy->new_call_details); - gpr_free(proxy); + grpc_core::Delete(proxy); } static void unrefpc(proxy_call* pc, const char* reason) { -- cgit v1.2.3 From dd95194a086b81966fd94726c04f53d279e247d8 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 4 Sep 2018 16:26:14 -0700 Subject: Prefer grpc status over http status test --- src/core/ext/transport/chttp2/transport/parsing.cc | 2 ++ src/core/lib/surface/call.cc | 4 ++-- test/core/end2end/tests/filter_status_code.cc | 20 +++++++++++++++++++- 3 files changed, 23 insertions(+), 3 deletions(-) (limited to 'test/core/end2end') diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 1e491d2ef8..205fb8c370 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -393,6 +393,7 @@ error_handler: static void free_timeout(void* p) { gpr_free(p); } static void on_initial_header(void* tp, grpc_mdelem md) { + gpr_log(GPR_INFO, "on initial header"); GPR_TIMER_SCOPE("on_initial_header", 0); grpc_chttp2_transport* t = static_cast(tp); @@ -475,6 +476,7 @@ static void on_initial_header(void* tp, grpc_mdelem md) { } static void on_trailing_header(void* tp, grpc_mdelem md) { + gpr_log(GPR_INFO, "on_trailing_header"); GPR_TIMER_SCOPE("on_trailing_header", 0); grpc_chttp2_transport* t = static_cast(tp); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index b07c4d6c10..496de1150a 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -685,10 +685,10 @@ static void cancel_with_status(grpc_call* c, grpc_status_code status, } static void set_final_status(grpc_call* call, grpc_error* error) { - if (grpc_call_error_trace.enabled()) { + //if (grpc_call_error_trace.enabled()) { gpr_log(GPR_DEBUG, "set_final_status %s", call->is_client ? "CLI" : "SVR"); gpr_log(GPR_DEBUG, "%s", grpc_error_string(error)); - } + //} if (call->is_client) { grpc_error_get_status(error, call->send_deadline, call->final_op.client.status, diff --git a/test/core/end2end/tests/filter_status_code.cc b/test/core/end2end/tests/filter_status_code.cc index ba3cbfa6d1..447ff520ee 100644 --- a/test/core/end2end/tests/filter_status_code.cc +++ b/test/core/end2end/tests/filter_status_code.cc @@ -249,6 +249,24 @@ typedef struct final_status_data { grpc_call_stack* call; } final_status_data; +static void start_transport_stream_op_batch(grpc_call_element *elem, + grpc_transport_stream_op_batch *op) { + auto* data = static_cast(elem->call_data); + if(data->call == g_server_call_stack) { + gpr_log(GPR_INFO, "here"); + } + if(op->send_initial_metadata) { + auto *batch = op->payload->send_initial_metadata.send_initial_metadata; + gpr_log(GPR_INFO, "init %p %p", batch->idx.named.status, batch->idx.named.grpc_status); + grpc_metadata_batch_substitute(batch, batch->idx.named.status, GRPC_MDELEM_STATUS_404); + } + if(op->send_trailing_metadata) { + auto *batch = op->payload->send_trailing_metadata.send_trailing_metadata; + gpr_log(GPR_INFO, "trai %p %p", batch->idx.named.status, batch->idx.named.grpc_status); + } + grpc_call_next_op(elem, op); +} + static grpc_error* init_call_elem(grpc_call_element* elem, const grpc_call_element_args* args) { final_status_data* data = static_cast(elem->call_data); @@ -307,7 +325,7 @@ static const grpc_channel_filter test_client_filter = { "client_filter_status_code"}; static const grpc_channel_filter test_server_filter = { - grpc_call_next_op, + start_transport_stream_op_batch, grpc_channel_next_op, sizeof(final_status_data), init_call_elem, -- cgit v1.2.3 From 3a41245e465e176dc2cae642cf701f5b476188b6 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 4 Sep 2018 19:18:15 -0700 Subject: Rectify the condition and add a test --- .../ext/filters/http/client/http_client_filter.cc | 7 ++++- src/core/ext/transport/chttp2/transport/parsing.cc | 2 -- src/core/lib/surface/call.cc | 4 +-- test/core/end2end/tests/filter_status_code.cc | 34 +++++++++++++--------- 4 files changed, 28 insertions(+), 19 deletions(-) (limited to 'test/core/end2end') diff --git a/src/core/ext/filters/http/client/http_client_filter.cc b/src/core/ext/filters/http/client/http_client_filter.cc index f44dc032a7..91fa163fec 100644 --- a/src/core/ext/filters/http/client/http_client_filter.cc +++ b/src/core/ext/filters/http/client/http_client_filter.cc @@ -79,7 +79,12 @@ struct channel_data { static grpc_error* client_filter_incoming_metadata(grpc_call_element* elem, grpc_metadata_batch* b) { if (b->idx.named.status != nullptr) { - if (grpc_mdelem_eq(b->idx.named.status->md, GRPC_MDELEM_STATUS_200)) { + /* If both gRPC status and HTTP status are provided in the response, we + * should prefer the gRPC status code, as mentioned in + * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. + */ + if (b->idx.named.grpc_status != nullptr || + grpc_mdelem_eq(b->idx.named.status->md, GRPC_MDELEM_STATUS_200)) { grpc_metadata_batch_remove(b, b->idx.named.status); } else { char* val = grpc_dump_slice(GRPC_MDVALUE(b->idx.named.status->md), diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 205fb8c370..1e491d2ef8 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -393,7 +393,6 @@ error_handler: static void free_timeout(void* p) { gpr_free(p); } static void on_initial_header(void* tp, grpc_mdelem md) { - gpr_log(GPR_INFO, "on initial header"); GPR_TIMER_SCOPE("on_initial_header", 0); grpc_chttp2_transport* t = static_cast(tp); @@ -476,7 +475,6 @@ static void on_initial_header(void* tp, grpc_mdelem md) { } static void on_trailing_header(void* tp, grpc_mdelem md) { - gpr_log(GPR_INFO, "on_trailing_header"); GPR_TIMER_SCOPE("on_trailing_header", 0); grpc_chttp2_transport* t = static_cast(tp); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 496de1150a..b07c4d6c10 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -685,10 +685,10 @@ static void cancel_with_status(grpc_call* c, grpc_status_code status, } static void set_final_status(grpc_call* call, grpc_error* error) { - //if (grpc_call_error_trace.enabled()) { + if (grpc_call_error_trace.enabled()) { gpr_log(GPR_DEBUG, "set_final_status %s", call->is_client ? "CLI" : "SVR"); gpr_log(GPR_DEBUG, "%s", grpc_error_string(error)); - //} + } if (call->is_client) { grpc_error_get_status(error, call->send_deadline, call->final_op.client.status, diff --git a/test/core/end2end/tests/filter_status_code.cc b/test/core/end2end/tests/filter_status_code.cc index 447ff520ee..5ffc3d00a3 100644 --- a/test/core/end2end/tests/filter_status_code.cc +++ b/test/core/end2end/tests/filter_status_code.cc @@ -16,6 +16,14 @@ * */ +/* This test verifies - + * 1) grpc_call_final_info passed to the filters on destroying a call contains + * the proper status. + * 2) If the response has both an HTTP status code and a gRPC status code, then + * we should prefer the gRPC status code as mentioned in + * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md + */ + #include "test/core/end2end/end2end_tests.h" #include @@ -249,20 +257,18 @@ typedef struct final_status_data { grpc_call_stack* call; } final_status_data; -static void start_transport_stream_op_batch(grpc_call_element *elem, - grpc_transport_stream_op_batch *op) { +static void server_start_transport_stream_op_batch( + grpc_call_element* elem, grpc_transport_stream_op_batch* op) { auto* data = static_cast(elem->call_data); - if(data->call == g_server_call_stack) { - gpr_log(GPR_INFO, "here"); - } - if(op->send_initial_metadata) { - auto *batch = op->payload->send_initial_metadata.send_initial_metadata; - gpr_log(GPR_INFO, "init %p %p", batch->idx.named.status, batch->idx.named.grpc_status); - grpc_metadata_batch_substitute(batch, batch->idx.named.status, GRPC_MDELEM_STATUS_404); - } - if(op->send_trailing_metadata) { - auto *batch = op->payload->send_trailing_metadata.send_trailing_metadata; - gpr_log(GPR_INFO, "trai %p %p", batch->idx.named.status, batch->idx.named.grpc_status); + if (data->call == g_server_call_stack) { + if (op->send_initial_metadata) { + auto* batch = op->payload->send_initial_metadata.send_initial_metadata; + if (batch->idx.named.status != nullptr) { + /* Replace the HTTP status with 404 */ + grpc_metadata_batch_substitute(batch, batch->idx.named.status, + GRPC_MDELEM_STATUS_404); + } + } } grpc_call_next_op(elem, op); } @@ -325,7 +331,7 @@ static const grpc_channel_filter test_client_filter = { "client_filter_status_code"}; static const grpc_channel_filter test_server_filter = { - start_transport_stream_op_batch, + server_start_transport_stream_op_batch, grpc_channel_next_op, sizeof(final_status_data), init_call_elem, -- cgit v1.2.3