Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/9601 )
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code ...................................................................... Patch Set 3: (2 comments) Thanks for the comments, looking into a better test. http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc-test.cc@724 PS3, Line 724: s.resize(TransferLimits::kMaxTotalSidecarBytes, 'a'); > can we add a test like this that actually sends and receives a valid sideca I will look into what test could exist. The awkward thing is that maxing out the sidecars to kMaxTotalSidecarBytes/INT_MAX would result in a message > INT_MAX, and the maximum value for rpc_max_message_size is INT_MAX, so the receiver would always reject it. But if I don't exceed INT_MAX for the message size, then I'm not getting test coverage. I could make rpc_max_message_size a larger type, but we don't really want to support larger message sizes. The code change here is fixing somewhat hypothetical issues. http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_sidecar.cc File src/kudu/rpc/rpc_sidecar.cc: http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_sidecar.cc@78 PS3, Line 78: if (buffer.size() > TransferLimits::kMaxTotalSidecarBytes) { > is kMaxTotalSidecarBytes meant to limit the total size of sidecars? Or the kMaxTotalSidecarBytes limits only sidecars. (I found it hard to know the header/pb sizes in the right places to enforce it correctly.) This particular code is looking only at the sidecar portion of the message. serialization::ParseMessage() strips off the header/pb and returns the sidecar-only part of the message. That is passed in here. (See InboundCall:ParseFrom()) -- To view, visit http://gerrit.cloudera.org:8080/9601 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24 Gerrit-Change-Number: 9601 Gerrit-PatchSet: 3 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: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 16 Mar 2018 21:29:01 +0000 Gerrit-HasComments: Yes
