From 5f3cfe960f708a397c4465a3064072dd3b2d7bb7 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 20 Jan 2017 11:02:11 -0500 Subject: Fix read from uninitialized memory bug in GrpcBufferWriter. This commit fixes an issue in which the following sequence of operations leads to use of uninitialized memory: 1. Caller invokes GrpcBufferWriter::Next(), and then makes use of 8191 bytes in the returned buffer (which is 8192 bytes in size). 2. Caller then returns the unused single byte via GrpcBufferWriter::BackUp(). This method invokes g_core_codegen_interface->grpc_slice_split_tail(), which causes backup_slice_ to be a grpc_slice with one byte. 3. At the next invocation of GrpcBufferWriter::Next(), a reference to the single byte grpc_slice is returned to the caller. The problem here is that the returned reference is to the inlined buffer in the grpc_slice, which is resident in slice_, not the location of the buffer inside slice_buffer_ after g_core_codegen_interface->grpc_slice_buffer_add() in GrpcBufferWriter::Next(). As a result, any data the caller writes to the returned void* data is lost. The solution is to avoid inlined backup slices. --- CMakeLists.txt | 32 +++ Makefile | 48 ++++ build.yaml | 12 + include/grpc++/impl/codegen/proto_utils.h | 9 +- test/cpp/codegen/codegen_test_full.cc | 2 +- test/cpp/codegen/proto_utils_test.cc | 93 ++++++++ tools/run_tests/generated/sources_and_headers.json | 17 ++ tools/run_tests/generated/tests.json | 22 ++ .../test/proto_utils_test/proto_utils_test.vcxproj | 248 +++++++++++++++++++++ .../proto_utils_test.vcxproj.filters | 200 +++++++++++++++++ 10 files changed, 681 insertions(+), 2 deletions(-) create mode 100644 test/cpp/codegen/proto_utils_test.cc create mode 100644 vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj create mode 100644 vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj.filters diff --git a/CMakeLists.txt b/CMakeLists.txt index d52e199842..c6a57e5f82 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -611,6 +611,7 @@ add_dependencies(buildtests_cxx metrics_client) add_dependencies(buildtests_cxx mock_test) add_dependencies(buildtests_cxx noop-benchmark) add_dependencies(buildtests_cxx proto_server_reflection_test) +add_dependencies(buildtests_cxx proto_utils_test) if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) add_dependencies(buildtests_cxx qps_interarrival_test) endif() @@ -8484,6 +8485,37 @@ target_link_libraries(proto_server_reflection_test ${_gRPC_GFLAGS_LIBRARIES} ) +endif (gRPC_BUILD_TESTS) +if (gRPC_BUILD_TESTS) + +add_executable(proto_utils_test + test/cpp/codegen/proto_utils_test.cc + third_party/googletest/src/gtest-all.cc +) + + +target_include_directories(proto_utils_test + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include + PRIVATE ${BORINGSSL_ROOT_DIR}/include + PRIVATE ${PROTOBUF_ROOT_DIR}/src + PRIVATE ${BENCHMARK_ROOT_DIR}/include + PRIVATE ${ZLIB_ROOT_DIR} + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/zlib + PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/third_party/gflags/include + PRIVATE third_party/googletest/include + PRIVATE third_party/googletest + PRIVATE ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(proto_utils_test + ${_gRPC_PROTOBUF_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc++ + grpc + ${_gRPC_GFLAGS_LIBRARIES} +) + endif (gRPC_BUILD_TESTS) if (gRPC_BUILD_TESTS) if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) diff --git a/Makefile b/Makefile index 0413cc0e00..d6b7a659be 100644 --- a/Makefile +++ b/Makefile @@ -1072,6 +1072,7 @@ metrics_client: $(BINDIR)/$(CONFIG)/metrics_client mock_test: $(BINDIR)/$(CONFIG)/mock_test noop-benchmark: $(BINDIR)/$(CONFIG)/noop-benchmark proto_server_reflection_test: $(BINDIR)/$(CONFIG)/proto_server_reflection_test +proto_utils_test: $(BINDIR)/$(CONFIG)/proto_utils_test qps_interarrival_test: $(BINDIR)/$(CONFIG)/qps_interarrival_test qps_json_driver: $(BINDIR)/$(CONFIG)/qps_json_driver qps_openloop_test: $(BINDIR)/$(CONFIG)/qps_openloop_test @@ -1469,6 +1470,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/mock_test \ $(BINDIR)/$(CONFIG)/noop-benchmark \ $(BINDIR)/$(CONFIG)/proto_server_reflection_test \ + $(BINDIR)/$(CONFIG)/proto_utils_test \ $(BINDIR)/$(CONFIG)/qps_interarrival_test \ $(BINDIR)/$(CONFIG)/qps_json_driver \ $(BINDIR)/$(CONFIG)/qps_openloop_test \ @@ -1572,6 +1574,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/mock_test \ $(BINDIR)/$(CONFIG)/noop-benchmark \ $(BINDIR)/$(CONFIG)/proto_server_reflection_test \ + $(BINDIR)/$(CONFIG)/proto_utils_test \ $(BINDIR)/$(CONFIG)/qps_interarrival_test \ $(BINDIR)/$(CONFIG)/qps_json_driver \ $(BINDIR)/$(CONFIG)/qps_openloop_test \ @@ -1901,6 +1904,8 @@ test_cxx: buildtests_cxx $(Q) $(BINDIR)/$(CONFIG)/noop-benchmark || ( echo test noop-benchmark failed ; exit 1 ) $(E) "[RUN] Testing proto_server_reflection_test" $(Q) $(BINDIR)/$(CONFIG)/proto_server_reflection_test || ( echo test proto_server_reflection_test failed ; exit 1 ) + $(E) "[RUN] Testing proto_utils_test" + $(Q) $(BINDIR)/$(CONFIG)/proto_utils_test || ( echo test proto_utils_test failed ; exit 1 ) $(E) "[RUN] Testing qps_openloop_test" $(Q) $(BINDIR)/$(CONFIG)/qps_openloop_test || ( echo test qps_openloop_test failed ; exit 1 ) $(E) "[RUN] Testing round_robin_end2end_test" @@ -13907,6 +13912,49 @@ endif endif +PROTO_UTILS_TEST_SRC = \ + test/cpp/codegen/proto_utils_test.cc \ + +PROTO_UTILS_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(PROTO_UTILS_TEST_SRC)))) +ifeq ($(NO_SECURE),true) + +# You can't build secure targets if you don't have OpenSSL. + +$(BINDIR)/$(CONFIG)/proto_utils_test: openssl_dep_error + +else + + + + +ifeq ($(NO_PROTOBUF),true) + +# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.0.0+. + +$(BINDIR)/$(CONFIG)/proto_utils_test: protobuf_dep_error + +else + +$(BINDIR)/$(CONFIG)/proto_utils_test: $(PROTOBUF_DEP) $(PROTO_UTILS_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LDXX) $(LDFLAGS) $(PROTO_UTILS_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/proto_utils_test + +endif + +endif + +$(OBJDIR)/$(CONFIG)/test/cpp/codegen/proto_utils_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a + +deps_proto_utils_test: $(PROTO_UTILS_TEST_OBJS:.o=.dep) + +ifneq ($(NO_SECURE),true) +ifneq ($(NO_DEPS),true) +-include $(PROTO_UTILS_TEST_OBJS:.o=.dep) +endif +endif + + QPS_INTERARRIVAL_TEST_SRC = \ test/cpp/qps/qps_interarrival_test.cc \ diff --git a/build.yaml b/build.yaml index 456db28cef..cacb5c4790 100644 --- a/build.yaml +++ b/build.yaml @@ -3442,6 +3442,18 @@ targets: - grpc - gpr_test_util - gpr +- name: proto_utils_test + gtest: true + build: test + language: c++ + src: + - test/cpp/codegen/proto_utils_test.cc + deps: + - grpc++ + - grpc + filegroups: + - grpc++_codegen_base + - grpc++_codegen_proto - name: qps_interarrival_test build: test run: false diff --git a/include/grpc++/impl/codegen/proto_utils.h b/include/grpc++/impl/codegen/proto_utils.h index 2123b62ed9..6df9de4fd2 100644 --- a/include/grpc++/impl/codegen/proto_utils.h +++ b/include/grpc++/impl/codegen/proto_utils.h @@ -50,6 +50,8 @@ extern CoreCodegenInterface* g_core_codegen_interface; namespace internal { +class GrpcBufferWriterPeer; + const int kGrpcBufferWriterMaxBufferLength = 8192; class GrpcBufferWriter final @@ -91,13 +93,18 @@ class GrpcBufferWriter final &slice_, GRPC_SLICE_LENGTH(slice_) - count); g_core_codegen_interface->grpc_slice_buffer_add(slice_buffer_, slice_); } - have_backup_ = true; + // It's dangerous to keep an inlined grpc_slice as the backup slice, since + // on a following Next() call, a reference will be returned to this slice + // via GRPC_SLICE_START_PTR, which will not be an adddress held by + // slice_buffer_. + have_backup_ = backup_slice_.refcount != NULL; byte_count_ -= count; } grpc::protobuf::int64 ByteCount() const override { return byte_count_; } private: + friend class GrpcBufferWriterPeer; const int block_size_; int64_t byte_count_; grpc_slice_buffer* slice_buffer_; diff --git a/test/cpp/codegen/codegen_test_full.cc b/test/cpp/codegen/codegen_test_full.cc index d6e2416b55..bc19fc9669 100644 --- a/test/cpp/codegen/codegen_test_full.cc +++ b/test/cpp/codegen/codegen_test_full.cc @@ -1,6 +1,6 @@ /* * - * Copyright 2016, Google Inc. + * Copyright 2017, Google Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without diff --git a/test/cpp/codegen/proto_utils_test.cc b/test/cpp/codegen/proto_utils_test.cc new file mode 100644 index 0000000000..1daa142b50 --- /dev/null +++ b/test/cpp/codegen/proto_utils_test.cc @@ -0,0 +1,93 @@ +/* + * + * Copyright 2017, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#include +#include +#include + +namespace grpc { +namespace internal { + +static GrpcLibraryInitializer g_gli_initializer; + +// Provide access to GrpcBufferWriter internals. +class GrpcBufferWriterPeer { + public: + explicit GrpcBufferWriterPeer(internal::GrpcBufferWriter* writer) + : writer_(writer) {} + bool have_backup() const { return writer_->have_backup_; } + const grpc_slice& backup_slice() const { return writer_->backup_slice_; } + const grpc_slice& slice() const { return writer_->slice_; } + + private: + GrpcBufferWriter* writer_; +}; + +class ProtoUtilsTest : public ::testing::Test {}; + +// Regression test for a memory corruption bug where a series of +// GrpcBufferWriter Next()/Backup() invocations could result in a dangling +// pointer returned by Next() due to the interaction between grpc_slice inlining +// and GRPC_SLICE_START_PTR. +TEST_F(ProtoUtilsTest, BackupNext) { + // Ensure the GrpcBufferWriter internals are initialized. + g_gli_initializer.summon(); + + grpc_byte_buffer* bp; + GrpcBufferWriter writer(&bp, 8192); + GrpcBufferWriterPeer peer(&writer); + + void* data; + int size; + // Allocate a slice. + ASSERT_TRUE(writer.Next(&data, &size)); + EXPECT_EQ(8192, size); + // Return a single byte. Before the fix that this test acts as a regression + // for, this would have resulted in an inlined backup slice. + writer.BackUp(1); + EXPECT_TRUE(!peer.have_backup()); + // On the next allocation, the slice is non-inlined. + ASSERT_TRUE(writer.Next(&data, &size)); + EXPECT_TRUE(peer.slice().refcount != NULL); + + // Cleanup. + g_core_codegen_interface->grpc_byte_buffer_destroy(bp); +} + +} // namespace internal +} // namespace grpc + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 52d8ae183b..def360afc8 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3048,6 +3048,23 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "grpc", + "grpc++", + "grpc++_codegen_base", + "grpc++_codegen_proto" + ], + "headers": [], + "is_filegroup": false, + "language": "c++", + "name": "proto_utils_test", + "src": [ + "test/cpp/codegen/proto_utils_test.cc" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "gpr", diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 4cc543962e..1b636686cd 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -2975,6 +2975,28 @@ "windows" ] }, + { + "args": [], + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "proto_utils_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ] + }, { "args": [], "ci_platforms": [ diff --git a/vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj b/vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj new file mode 100644 index 0000000000..fc2355624f --- /dev/null +++ b/vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj @@ -0,0 +1,248 @@ + + + + + + Debug + Win32 + + + Debug + x64 + + + Release + Win32 + + + Release + x64 + + + + {D55F5F6B-BAB1-C421-7138-CDC08C686DCF} + true + $(SolutionDir)IntDir\$(MSBuildProjectName)\ + + + + v100 + + + v110 + + + v120 + + + v140 + + + Application + true + Unicode + + + Application + false + true + Unicode + + + + + + + + + + + + + + + + proto_utils_test + static + Debug + static + Debug + + + proto_utils_test + static + Release + static + Release + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + {C187A093-A0FE-489D-A40A-6E33DE0F9FEB} + + + {29D16885-7228-4C31-81ED-5F9187C7F2A9} + + + + + + + + + + + + + + + This project references NuGet package(s) that are missing on this computer. Enable NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}. + + + + + + + + + diff --git a/vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj.filters b/vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj.filters new file mode 100644 index 0000000000..e6b4a92458 --- /dev/null +++ b/vsprojects/vcxproj/test/proto_utils_test/proto_utils_test.vcxproj.filters @@ -0,0 +1,200 @@ + + + + + test\cpp\codegen + + + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen\security + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc\impl\codegen + + + include\grpc++\impl\codegen + + + include\grpc++\impl\codegen + + + + + + {cc3989e4-cb32-e89b-2d05-41ad6dd1dfdf} + + + {59dc67c7-fb18-960b-db77-99ea8e6c0a5f} + + + {6c2c28e0-a57f-8eb7-4897-41ec191373f9} + + + {6740e44a-b709-0cb2-faa4-c1a792d909c5} + + + {cbb79e1d-95bc-e475-8e86-af33a7603af1} + + + {c4a45c0f-ccce-7e26-d0bb-9e54ad676140} + + + {4aff0d7b-5c9d-080c-faa6-e8126853f149} + + + {35a120bf-8b2f-77e1-cab7-24259b5ed1db} + + + {9d60ac43-ff38-4dfc-8c33-9167fb47fa90} + + + {7845357d-2b4b-6f25-c68b-79be9ab587fb} + + + {22e8c70f-796c-f2c0-d87d-e4ef419609a1} + + + + -- cgit v1.2.3