From 62f28bfcf0cdda607278542566ddfc0a7cbd9f00 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 19 Jan 2017 10:05:13 -0800 Subject: [PATCH 1/2] Remove double-checking of max-message-size in C++ layers --- include/grpc++/impl/codegen/call.h | 71 ++++++------------- .../grpc++/impl/codegen/method_handler_impl.h | 8 +-- include/grpc++/impl/codegen/proto_utils.h | 8 +-- .../grpc++/impl/codegen/rpc_service_method.h | 9 +-- .../grpc++/impl/codegen/server_interface.h | 4 +- include/grpc++/impl/codegen/sync_stream.h | 8 +-- include/grpc++/support/byte_buffer.h | 3 +- src/cpp/server/server_cc.cc | 8 +-- 8 files changed, 40 insertions(+), 79 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 6ab00612f6e..9b86fd608f0 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -175,7 +175,7 @@ template class CallNoOp { protected: void AddOp(grpc_op* ops, size_t* nops) {} - void FinishOp(bool* status, int max_receive_message_size) {} + void FinishOp(bool* status) {} }; class CallOpSendInitialMetadata { @@ -213,7 +213,7 @@ class CallOpSendInitialMetadata { op->data.send_initial_metadata.maybe_compression_level.level = maybe_compression_level_.level; } - void FinishOp(bool* status, int max_receive_message_size) { + void FinishOp(bool* status) { if (!send_) return; g_core_codegen_interface->gpr_free(initial_metadata_); send_ = false; @@ -253,7 +253,7 @@ class CallOpSendMessage { // Flags are per-message: clear them after use. write_options_.Clear(); } - void FinishOp(bool* status, int max_receive_message_size) { + void FinishOp(bool* status) { if (own_buf_) g_core_codegen_interface->grpc_byte_buffer_destroy(send_buf_); send_buf_ = nullptr; } @@ -301,14 +301,12 @@ class CallOpRecvMessage { op->data.recv_message = &recv_buf_; } - void FinishOp(bool* status, int max_receive_message_size) { + void FinishOp(bool* status) { if (message_ == nullptr) return; if (recv_buf_) { if (*status) { got_message = *status = - SerializationTraits::Deserialize(recv_buf_, message_, - max_receive_message_size) - .ok(); + SerializationTraits::Deserialize(recv_buf_, message_).ok(); } else { got_message = false; g_core_codegen_interface->grpc_byte_buffer_destroy(recv_buf_); @@ -331,8 +329,7 @@ class CallOpRecvMessage { namespace CallOpGenericRecvMessageHelper { class DeserializeFunc { public: - virtual Status Deserialize(grpc_byte_buffer* buf, - int max_receive_message_size) = 0; + virtual Status Deserialize(grpc_byte_buffer* buf) = 0; virtual ~DeserializeFunc() {} }; @@ -340,10 +337,8 @@ template class DeserializeFuncType final : public DeserializeFunc { public: DeserializeFuncType(R* message) : message_(message) {} - Status Deserialize(grpc_byte_buffer* buf, - int max_receive_message_size) override { - return SerializationTraits::Deserialize(buf, message_, - max_receive_message_size); + Status Deserialize(grpc_byte_buffer* buf) override { + return SerializationTraits::Deserialize(buf, message_); } ~DeserializeFuncType() override {} @@ -382,13 +377,12 @@ class CallOpGenericRecvMessage { op->data.recv_message = &recv_buf_; } - void FinishOp(bool* status, int max_receive_message_size) { + void FinishOp(bool* status) { if (!deserialize_) return; if (recv_buf_) { if (*status) { got_message = true; - *status = - deserialize_->Deserialize(recv_buf_, max_receive_message_size).ok(); + *status = deserialize_->Deserialize(recv_buf_).ok(); } else { got_message = false; g_core_codegen_interface->grpc_byte_buffer_destroy(recv_buf_); @@ -422,7 +416,7 @@ class CallOpClientSendClose { op->flags = 0; op->reserved = NULL; } - void FinishOp(bool* status, int max_receive_message_size) { send_ = false; } + void FinishOp(bool* status) { send_ = false; } private: bool send_; @@ -457,7 +451,7 @@ class CallOpServerSendStatus { op->reserved = NULL; } - void FinishOp(bool* status, int max_receive_message_size) { + void FinishOp(bool* status) { if (!send_status_available_) return; g_core_codegen_interface->gpr_free(trailing_metadata_); send_status_available_ = false; @@ -490,7 +484,7 @@ class CallOpRecvInitialMetadata { op->flags = 0; op->reserved = NULL; } - void FinishOp(bool* status, int max_receive_message_size) { + void FinishOp(bool* status) { if (recv_initial_metadata_ == nullptr) return; FillMetadataMap(&recv_initial_metadata_arr_, recv_initial_metadata_); recv_initial_metadata_ = nullptr; @@ -529,7 +523,7 @@ class CallOpClientRecvStatus { op->reserved = NULL; } - void FinishOp(bool* status, int max_receive_message_size) { + void FinishOp(bool* status) { if (recv_status_ == nullptr) return; FillMetadataMap(&recv_trailing_metadata_arr_, recv_trailing_metadata_); *recv_status_ = Status( @@ -566,22 +560,17 @@ class CallOpSetCollectionInterface /// API. class CallOpSetInterface : public CompletionQueueTag { public: - CallOpSetInterface() : max_receive_message_size_(0) {} + CallOpSetInterface() {} /// Fills in grpc_op, starting from ops[*nops] and moving /// upwards. virtual void FillOps(grpc_op* ops, size_t* nops) = 0; - void set_max_receive_message_size(int max_receive_message_size) { - max_receive_message_size_ = max_receive_message_size; - } - /// Mark this as belonging to a collection if needed void SetCollection(std::shared_ptr collection) { collection_ = collection; } protected: - int max_receive_message_size_; std::shared_ptr collection_; }; @@ -613,12 +602,12 @@ class CallOpSet : public CallOpSetInterface, } bool FinalizeResult(void** tag, bool* status) override { - this->Op1::FinishOp(status, max_receive_message_size_); - this->Op2::FinishOp(status, max_receive_message_size_); - this->Op3::FinishOp(status, max_receive_message_size_); - this->Op4::FinishOp(status, max_receive_message_size_); - this->Op5::FinishOp(status, max_receive_message_size_); - this->Op6::FinishOp(status, max_receive_message_size_); + this->Op1::FinishOp(status); + this->Op2::FinishOp(status); + this->Op3::FinishOp(status); + this->Op4::FinishOp(status); + this->Op5::FinishOp(status); + this->Op6::FinishOp(status); *tag = return_tag_; collection_.reset(); // drop the ref at this point return true; @@ -650,35 +639,19 @@ class Call final { public: /* call is owned by the caller */ Call(grpc_call* call, CallHook* call_hook, CompletionQueue* cq) - : call_hook_(call_hook), - cq_(cq), - call_(call), - max_receive_message_size_(-1) {} - - Call(grpc_call* call, CallHook* call_hook, CompletionQueue* cq, - int max_receive_message_size) - : call_hook_(call_hook), - cq_(cq), - call_(call), - max_receive_message_size_(max_receive_message_size) {} + : call_hook_(call_hook), cq_(cq), call_(call) {} void PerformOps(CallOpSetInterface* ops) { - if (max_receive_message_size_ > 0) { - ops->set_max_receive_message_size(max_receive_message_size_); - } call_hook_->PerformOpsOnCall(ops, this); } grpc_call* call() const { return call_; } CompletionQueue* cq() const { return cq_; } - int max_receive_message_size() { return max_receive_message_size_; } - private: CallHook* call_hook_; CompletionQueue* cq_; grpc_call* call_; - int max_receive_message_size_; }; } // namespace grpc diff --git a/include/grpc++/impl/codegen/method_handler_impl.h b/include/grpc++/impl/codegen/method_handler_impl.h index d5d27e15cd6..83b569ce74f 100644 --- a/include/grpc++/impl/codegen/method_handler_impl.h +++ b/include/grpc++/impl/codegen/method_handler_impl.h @@ -52,8 +52,8 @@ class RpcMethodHandler : public MethodHandler { void RunHandler(const HandlerParameter& param) final { RequestType req; - Status status = SerializationTraits::Deserialize( - param.request, &req, param.max_receive_message_size); + Status status = + SerializationTraits::Deserialize(param.request, &req); ResponseType rsp; if (status.ok()) { status = func_(service_, param.server_context, &req, &rsp); @@ -138,8 +138,8 @@ class ServerStreamingHandler : public MethodHandler { void RunHandler(const HandlerParameter& param) final { RequestType req; - Status status = SerializationTraits::Deserialize( - param.request, &req, param.max_receive_message_size); + Status status = + SerializationTraits::Deserialize(param.request, &req); if (status.ok()) { ServerWriter writer(param.call, param.server_context); diff --git a/include/grpc++/impl/codegen/proto_utils.h b/include/grpc++/impl/codegen/proto_utils.h index 2f154875916..acaa48afee1 100644 --- a/include/grpc++/impl/codegen/proto_utils.h +++ b/include/grpc++/impl/codegen/proto_utils.h @@ -203,8 +203,7 @@ class SerializationTraits 0) { - decoder.SetTotalBytesLimit(max_receive_message_size, - max_receive_message_size); - } + decoder.SetTotalBytesLimit(UINT32_MAX, UINT32_MAX); if (!msg->ParseFromCodedStream(&decoder)) { result = Status(StatusCode::INTERNAL, msg->InitializationErrorString()); } diff --git a/include/grpc++/impl/codegen/rpc_service_method.h b/include/grpc++/impl/codegen/rpc_service_method.h index 78c54e37711..eb8f9a1096e 100644 --- a/include/grpc++/impl/codegen/rpc_service_method.h +++ b/include/grpc++/impl/codegen/rpc_service_method.h @@ -57,17 +57,12 @@ class MethodHandler { public: virtual ~MethodHandler() {} struct HandlerParameter { - HandlerParameter(Call* c, ServerContext* context, grpc_byte_buffer* req, - int max_size) - : call(c), - server_context(context), - request(req), - max_receive_message_size(max_size) {} + HandlerParameter(Call* c, ServerContext* context, grpc_byte_buffer* req) + : call(c), server_context(context), request(req) {} Call* call; ServerContext* server_context; // Handler required to grpc_byte_buffer_destroy this grpc_byte_buffer* request; - int max_receive_message_size; }; virtual void RunHandler(const HandlerParameter& param) = 0; }; diff --git a/include/grpc++/impl/codegen/server_interface.h b/include/grpc++/impl/codegen/server_interface.h index 666b9ff66eb..752e493a9eb 100644 --- a/include/grpc++/impl/codegen/server_interface.h +++ b/include/grpc++/impl/codegen/server_interface.h @@ -199,9 +199,7 @@ class ServerInterface : public CallHook { bool FinalizeResult(void** tag, bool* status) override { bool serialization_status = *status && payload_ && - SerializationTraits::Deserialize( - payload_, request_, server_->max_receive_message_size()) - .ok(); + SerializationTraits::Deserialize(payload_, request_).ok(); bool ret = RegisteredAsyncRequest::FinalizeResult(tag, status); *status = serialization_status && *status; return ret; diff --git a/include/grpc++/impl/codegen/sync_stream.h b/include/grpc++/impl/codegen/sync_stream.h index 4d9b074e95f..e0f9ba7701b 100644 --- a/include/grpc++/impl/codegen/sync_stream.h +++ b/include/grpc++/impl/codegen/sync_stream.h @@ -160,7 +160,7 @@ class ClientReader final : public ClientReaderInterface { } bool NextMessageSize(uint32_t* sz) override { - *sz = call_.max_receive_message_size(); + *sz = UINT32_MAX; return true; } @@ -310,7 +310,7 @@ class ClientReaderWriter final : public ClientReaderWriterInterface { } bool NextMessageSize(uint32_t* sz) override { - *sz = call_.max_receive_message_size(); + *sz = UINT32_MAX; return true; } @@ -382,7 +382,7 @@ class ServerReader final : public ServerReaderInterface { } bool NextMessageSize(uint32_t* sz) override { - *sz = call_->max_receive_message_size(); + *sz = UINT32_MAX; return true; } @@ -474,7 +474,7 @@ class ServerReaderWriterBody final { } bool NextMessageSize(uint32_t* sz) { - *sz = call_->max_receive_message_size(); + *sz = UINT32_MAX; return true; } diff --git a/include/grpc++/support/byte_buffer.h b/include/grpc++/support/byte_buffer.h index 1f317df6634..064a03b977b 100644 --- a/include/grpc++/support/byte_buffer.h +++ b/include/grpc++/support/byte_buffer.h @@ -95,8 +95,7 @@ class ByteBuffer final { template <> class SerializationTraits { public: - static Status Deserialize(grpc_byte_buffer* byte_buffer, ByteBuffer* dest, - int max_receive_message_size) { + static Status Deserialize(grpc_byte_buffer* byte_buffer, ByteBuffer* dest) { dest->set_buffer(byte_buffer); return Status::OK; } diff --git a/src/cpp/server/server_cc.cc b/src/cpp/server/server_cc.cc index 817d85a81ca..7785a1f1241 100644 --- a/src/cpp/server/server_cc.cc +++ b/src/cpp/server/server_cc.cc @@ -186,7 +186,7 @@ class Server::SyncRequest final : public CompletionQueueTag { public: explicit CallData(Server* server, SyncRequest* mrd) : cq_(mrd->cq_), - call_(mrd->call_, server, &cq_, server->max_receive_message_size_), + call_(mrd->call_, server, &cq_), ctx_(mrd->deadline_, mrd->request_metadata_.metadata, mrd->request_metadata_.count), has_request_payload_(mrd->has_request_payload_), @@ -208,8 +208,8 @@ class Server::SyncRequest final : public CompletionQueueTag { void Run(std::shared_ptr global_callbacks) { ctx_.BeginCompletionOp(&call_); global_callbacks->PreSynchronousRequest(&ctx_); - method_->handler()->RunHandler(MethodHandler::HandlerParameter( - &call_, &ctx_, request_payload_, call_.max_receive_message_size())); + method_->handler()->RunHandler( + MethodHandler::HandlerParameter(&call_, &ctx_, request_payload_)); global_callbacks->PostSynchronousRequest(&ctx_); request_payload_ = nullptr; void* ignored_tag; @@ -598,7 +598,7 @@ bool ServerInterface::BaseAsyncRequest::FinalizeResult(void** tag, grpc_metadata_array_destroy(&initial_metadata_array_); context_->set_call(call_); context_->cq_ = call_cq_; - Call call(call_, server_, call_cq_, server_->max_receive_message_size()); + Call call(call_, server_, call_cq_); if (*status && call_) { context_->BeginCompletionOp(&call); } From 1d77059e4b1ec17c704acfc876e321cc33bff99d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 19 Jan 2017 11:44:42 -0800 Subject: [PATCH 2/2] Fix integer overflow --- include/grpc++/impl/codegen/proto_utils.h | 2 +- include/grpc++/impl/codegen/sync_stream.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/grpc++/impl/codegen/proto_utils.h b/include/grpc++/impl/codegen/proto_utils.h index acaa48afee1..2123b62ed9e 100644 --- a/include/grpc++/impl/codegen/proto_utils.h +++ b/include/grpc++/impl/codegen/proto_utils.h @@ -214,7 +214,7 @@ class SerializationTraitsParseFromCodedStream(&decoder)) { result = Status(StatusCode::INTERNAL, msg->InitializationErrorString()); } diff --git a/include/grpc++/impl/codegen/sync_stream.h b/include/grpc++/impl/codegen/sync_stream.h index e0f9ba7701b..1f7708bab94 100644 --- a/include/grpc++/impl/codegen/sync_stream.h +++ b/include/grpc++/impl/codegen/sync_stream.h @@ -160,7 +160,7 @@ class ClientReader final : public ClientReaderInterface { } bool NextMessageSize(uint32_t* sz) override { - *sz = UINT32_MAX; + *sz = INT_MAX; return true; } @@ -310,7 +310,7 @@ class ClientReaderWriter final : public ClientReaderWriterInterface { } bool NextMessageSize(uint32_t* sz) override { - *sz = UINT32_MAX; + *sz = INT_MAX; return true; } @@ -382,7 +382,7 @@ class ServerReader final : public ServerReaderInterface { } bool NextMessageSize(uint32_t* sz) override { - *sz = UINT32_MAX; + *sz = INT_MAX; return true; } @@ -474,7 +474,7 @@ class ServerReaderWriterBody final { } bool NextMessageSize(uint32_t* sz) { - *sz = UINT32_MAX; + *sz = INT_MAX; return true; }