Joe McDonnell has posted comments on this change. ( )

Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages

Patch Set 1:


- Switched to using int64_t.
- ASAN requires explicit casts in the serialization code, so I added those.
- Modified rpc-test to avoid the git tidy issue. (There is a different way to 
fix it if that is desired.)
Commit Message:
PS1, Line 16:  - Local variables in SerializeMessage() become uint32_t
            :    rather than int.
> what about just using int64 for the variables? we try not to use unsigneds
PS1, Line 26: exceeding rpc_max_message_size (maximum INT_MAX).
> should we add validation to the flag to ensure that it isn't set to any val
If I'm understanding it correctly, I think the enforcement of 
rpc_max_message_size in InboundTransfer::ReceieveBuffer() is doing what we 
want. For a N byte message, the first 4 bytes are the size (which doesn't 
include itself, so is N-4). We add back the 4 bytes and do the check. If N > 
rpc_max_message_size, it will be rejected. So, even rpc_max_message_size = 
INT_MAX should be enforced correctly.

A sender can't successfully send a rpc_max_message_size payload due to the 
added header (50ish bytes).
File src/kudu/rpc/
PS1, Line 646:     string s(INT_MAX, 'a');
> warning: suspicious large length parameter [misc-string-constructor]
Bypassed this by doing a resize.

An alternative fix is to modify src/kudu/.clang_tidy to add this line (which 
disables the check for large string initialization):
  - key:             misc-string-constructor.WarnOnLargeLength
    value:           0
File src/kudu/rpc/
PS1, Line 59:   // KUDU-2305: Use unsigned 4-byte integers to avoid overflowing 
when additional_size
> this sort of makes it sound like you're pointing out an existing bug instea
Makes sense. Fixed.
PS1, Line 120:   DCHECK_EQ(total_len + kMsgLengthPrefixLength, buf.size())
> maybe safer here to subtract from buf.size rather than add to total_len, so

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c
Gerrit-Change-Number: 9355
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Mon, 19 Feb 2018 20:31:51 +0000
Gerrit-HasComments: Yes

Reply via email to