Todd Lipcon 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: (4 comments) 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 when possible 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 value larger than INT_MAX-4 or whatever the true maximum is? 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 instead of documenting that the fix is related to this JIRA. I think better to not reference the JIRA and if someone is confused they can find the JIRA via git blame 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 it can't overflow even if the caller sends UINT32_MAX -- 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: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 16 Feb 2018 23:27:09 +0000 Gerrit-HasComments: Yes
