[ 
https://issues.apache.org/jira/browse/KUDU-2305?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16396152#comment-16396152
 ] 

Michael Ho edited comment on KUDU-2305 at 3/12/18 10:14 PM:
------------------------------------------------------------

Won't checking at {{InboundCall::AddOutboundSidecar()}} / 
{{RpcController::AddOutboundSidecar()}} still miss the corner case in which the 
total message size (including the serialized request buffer) may exceed 
UINT32_MAX ? It may work if we limit the total size of all sidecars to UINT_MAX 
- max_serialized_buffer_size - max_header_size ?

It seems more natural to check at {{OutboundCall:SetRequestPayload()}} / 
{{InboundCall::SerializeResponseBuffer()}} except there doesn't currently seem 
to be a clean way to propagate the error. May be we can treat it as an RPC 
failure by invoking the callback on the OutboundCall's callback on the outbound 
path and return an RPC error to the client in the InboundCall response path ?
{quote}The rpc header encodes the sidecar offsets with a uint32. So, we know 
that it is impossible to correctly serialize a message larger than UINT_MAX
{quote}
Should we have a CHECK in {{SerializeHeader()}} for this ?

Anyhow, given the max message size in the header is UINT_MAX, we may still want 
to consider fixing some of those functions mentioned above to not use int32_t 
for total size.


was (Author: kwho):
Won't checking at {{InboundCall::AddOutboundSidecar()}} / 
{{RpcController::AddOutboundSidecar()}} still miss the corner case in which the 
total message size (including the serialized request buffer) may exceed 
UINT32_MAX ? It may work if we limit the total size of all sidecars to UINT_MAX 
- max_serialized_buffer_size - max_header_size ?

It seems more natural to check at {{OutboundCall:SetRequestPayload()}} / 
{{InboundCall::SerializeResponseBuffer()}} except there doesn't currently seem 
to be a clean way to propagate the error. May be we can treat it as an RPC 
failure by invoking the callback on the OutboundCall's callback on the outbound 
path and return an RPC error to the client in the InboundCall response path ?

bq. The rpc header encodes the sidecar offsets with a uint32. So, we know that 
it is impossible to correctly serialize a message larger than UINT_MAX

Should we have a CHECK in {{SerializeHeader()}} for this ?

> Local variables can overflow when serializing a 2GB message
> -----------------------------------------------------------
>
>                 Key: KUDU-2305
>                 URL: https://issues.apache.org/jira/browse/KUDU-2305
>             Project: Kudu
>          Issue Type: Bug
>          Components: rpc
>    Affects Versions: 1.6.0
>            Reporter: Joe McDonnell
>            Assignee: Joe McDonnell
>            Priority: Major
>             Fix For: 1.7.0
>
>
> When rpc_max_message_size is set to its maximum of INT_MAX (2147483647), 
> certain local variables in SerializeMessage can overflow as messages approach 
> this size. Specifically, recorded_size, size_with_delim, and total_size are 4 
> byte signed integers and could overflow when additional_size becomes large.
> Since INT_MAX is the largest allowable value for rpc_max_message_size (a 4 
> byte signed integer), these variables will not overflow if changed to 4 byte 
> unsigned integers. This would eliminate the potential problem for 
> serialization.
> A similar problem exists in the InboundTransfer::ReceiveBuffer() and similar 
> codepaths. Changing those variables to unsigned integers should resolve the 
> issue.
> This does not impact existing systems, because the default value of 
> rpc_max_message_size is 50MB.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to