Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/9355 )
Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages ...................................................................... Patch Set 1: (5 comments) - 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.) http://gerrit.cloudera.org:8080/#/c/9355/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9355/1//COMMIT_MSG@16 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 Fixed http://gerrit.cloudera.org:8080/#/c/9355/1//COMMIT_MSG@26 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). http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/rpc-test.cc@646 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 http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/serialization.cc File src/kudu/rpc/serialization.cc: http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/serialization.cc@59 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. http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/serialization.cc@120 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 Fixed. -- To view, visit http://gerrit.cloudera.org:8080/9355 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c Gerrit-Change-Number: 9355 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 19 Feb 2018 20:31:51 +0000 Gerrit-HasComments: Yes
