From 303d3082a07363c29dc747e986658fd6c8dc4053 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Thu, 5 May 2016 18:25:34 -0700 Subject: [PATCH 1/2] Fixed compression interop and re-enable for C++. Also added some defense in depth for compression algorithms in the receive path. --- src/core/lib/channel/compress_filter.c | 6 ++++-- src/core/lib/compression/message_compress.c | 2 +- src/core/lib/surface/byte_buffer_reader.c | 19 +++++++++++++------ src/core/lib/surface/call.c | 9 +++++++-- test/cpp/interop/interop_client.cc | 2 +- tools/run_tests/run_interop_tests.py | 4 ++-- 6 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/core/lib/channel/compress_filter.c b/src/core/lib/channel/compress_filter.c index 5510c79b183..9769070cc12 100644 --- a/src/core/lib/channel/compress_filter.c +++ b/src/core/lib/channel/compress_filter.c @@ -189,8 +189,10 @@ static void finish_send_message(grpc_exec_ctx *exec_ctx, char *algo_name; GPR_ASSERT(grpc_compression_algorithm_name(calld->compression_algorithm, &algo_name)); - gpr_log(GPR_DEBUG, "Algorithm '%s' enabled but decided not to compress.", - algo_name); + gpr_log( + GPR_DEBUG, + "Algorithm '%s' enabled but decided not to compress. Input size: %d", + algo_name, calld->slices.length); } } diff --git a/src/core/lib/compression/message_compress.c b/src/core/lib/compression/message_compress.c index cbe0b5a2856..699719a523d 100644 --- a/src/core/lib/compression/message_compress.c +++ b/src/core/lib/compression/message_compress.c @@ -194,5 +194,5 @@ int grpc_msg_decompress(grpc_compression_algorithm algorithm, break; } gpr_log(GPR_ERROR, "invalid compression algorithm %d", algorithm); - return 0; + return -1; /* to distinguish it from GRPC_COMPRESS_NONE */ } diff --git a/src/core/lib/surface/byte_buffer_reader.c b/src/core/lib/surface/byte_buffer_reader.c index 809fd5f1fa6..c7f941525df 100644 --- a/src/core/lib/surface/byte_buffer_reader.c +++ b/src/core/lib/surface/byte_buffer_reader.c @@ -62,12 +62,19 @@ void grpc_byte_buffer_reader_init(grpc_byte_buffer_reader *reader, case GRPC_BB_RAW: gpr_slice_buffer_init(&decompressed_slices_buffer); if (is_compressed(reader->buffer_in)) { - grpc_msg_decompress(reader->buffer_in->data.raw.compression, - &reader->buffer_in->data.raw.slice_buffer, - &decompressed_slices_buffer); - reader->buffer_out = - grpc_raw_byte_buffer_create(decompressed_slices_buffer.slices, - decompressed_slices_buffer.count); + if (grpc_msg_decompress(reader->buffer_in->data.raw.compression, + &reader->buffer_in->data.raw.slice_buffer, + &decompressed_slices_buffer) < 0) { + gpr_log(GPR_ERROR, + "Unexpected error decompressing data for algorithm with enum " + "value '%d'. Reading data as if it were uncompressed.", + reader->buffer_in->data.raw.compression); + reader->buffer_out = reader->buffer_in; + } else { /* all fine */ + reader->buffer_out = + grpc_raw_byte_buffer_create(decompressed_slices_buffer.slices, + decompressed_slices_buffer.count); + } gpr_slice_buffer_destroy(&decompressed_slices_buffer); } else { /* not compressed, use the input buffer as output */ reader->buffer_out = reader->buffer_in; diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 9b2b94eedf5..778557121d7 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -408,6 +408,7 @@ static void set_status_code(grpc_call *call, status_source source, static void set_compression_algorithm(grpc_call *call, grpc_compression_algorithm algo) { + GPR_ASSERT(algo < GRPC_COMPRESS_ALGORITHMS_COUNT); call->compression_algorithm = algo; } @@ -828,12 +829,16 @@ static uint32_t decode_status(grpc_mdelem *md) { return status; } -static uint32_t decode_compression(grpc_mdelem *md) { +static grpc_compression_algorithm decode_compression(grpc_mdelem *md) { grpc_compression_algorithm algorithm = grpc_compression_algorithm_from_mdstr(md->value); if (algorithm == GRPC_COMPRESS_ALGORITHMS_COUNT) { const char *md_c_str = grpc_mdstr_as_c_string(md->value); - gpr_log(GPR_ERROR, "Invalid compression algorithm: '%s'", md_c_str); + gpr_log(GPR_ERROR, + "Invalid incoming compression algorithm: '%s'. Interpreting " + "incoming data as uncompressed.", + md_c_str); + return GRPC_COMPRESS_NONE; } return algorithm; } diff --git a/test/cpp/interop/interop_client.cc b/test/cpp/interop/interop_client.cc index 22293d211fa..314d6c8eae2 100644 --- a/test/cpp/interop/interop_client.cc +++ b/test/cpp/interop/interop_client.cc @@ -60,7 +60,7 @@ static const char* kRandomFile = "test/cpp/interop/rnd.dat"; namespace { // The same value is defined by the Java client. const std::vector request_stream_sizes = {27182, 8, 1828, 45904}; -const std::vector response_stream_sizes = {31415, 9, 2653, 58979}; +const std::vector response_stream_sizes = {31415, 59, 2653, 58979}; const int kNumResponseMessages = 2000; const int kResponseMessageSize = 1030; const int kReceiveDelayMilliSeconds = 20; diff --git a/tools/run_tests/run_interop_tests.py b/tools/run_tests/run_interop_tests.py index e813473421d..edbdf05e2a2 100755 --- a/tools/run_tests/run_interop_tests.py +++ b/tools/run_tests/run_interop_tests.py @@ -82,10 +82,10 @@ class CXXLanguage: return {} def unimplemented_test_cases(self): - return _SKIP_ADVANCED + _SKIP_COMPRESSION + return _SKIP_ADVANCED def unimplemented_test_cases_server(self): - return _SKIP_ADVANCED + _SKIP_COMPRESSION + return _SKIP_ADVANCED def __str__(self): return 'c++' From 0b405d54d496acc331f1511353b26e9f9a0e4598 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 9 May 2016 15:58:22 -0700 Subject: [PATCH 2/2] fixed wrong change --- src/core/lib/compression/message_compress.c | 2 +- src/core/lib/surface/byte_buffer_reader.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/compression/message_compress.c b/src/core/lib/compression/message_compress.c index 699719a523d..cbe0b5a2856 100644 --- a/src/core/lib/compression/message_compress.c +++ b/src/core/lib/compression/message_compress.c @@ -194,5 +194,5 @@ int grpc_msg_decompress(grpc_compression_algorithm algorithm, break; } gpr_log(GPR_ERROR, "invalid compression algorithm %d", algorithm); - return -1; /* to distinguish it from GRPC_COMPRESS_NONE */ + return 0; } diff --git a/src/core/lib/surface/byte_buffer_reader.c b/src/core/lib/surface/byte_buffer_reader.c index c7f941525df..c97079f6385 100644 --- a/src/core/lib/surface/byte_buffer_reader.c +++ b/src/core/lib/surface/byte_buffer_reader.c @@ -64,7 +64,7 @@ void grpc_byte_buffer_reader_init(grpc_byte_buffer_reader *reader, if (is_compressed(reader->buffer_in)) { if (grpc_msg_decompress(reader->buffer_in->data.raw.compression, &reader->buffer_in->data.raw.slice_buffer, - &decompressed_slices_buffer) < 0) { + &decompressed_slices_buffer) == 0) { gpr_log(GPR_ERROR, "Unexpected error decompressing data for algorithm with enum " "value '%d'. Reading data as if it were uncompressed.",