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 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:

http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/inbound_call.h@253
PS1, Line 253: outbound_sidecars_size_
> I think 'total_size_' or 'size_bytes_' or something is more clear, since 's
Changed to "outbound_sidecars_total_bytes_"


http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/inbound_call.cc
File src/kudu/rpc/inbound_call.cc:

http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/inbound_call.cc@186
PS1, Line 186: absolute_sidecar_offset - protobuf_msg_size
> these are both uint32_t, right? maybe we should change absolute_sidecar_off
Good point, best to be defensive. Changed absolute_sidecar_offset to an int64_t 
and added a check for it be less than UINT_MAX.


http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/inbound_call.cc@221
PS1, Line 221: exceeds limit
> would exceed
Done


http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/rpc_controller.h@275
PS1, Line 275:   int64_t outbound_sidecars_size_ = 0;
> same comment as on the InboundCall
Done


http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/rpc_controller.cc
File src/kudu/rpc/rpc_controller.cc:

http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/rpc_controller.cc@153
PS1, Line 153: exceeds limit
> would exceed
Done


http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/transfer.h
File src/kudu/rpc/transfer.h:

http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/transfer.h@51
PS1, Line 51:     kMaxTotalSidecarSize = INT_MAX
> it seems elsewhere in this patch you are checking against INT_MAX instead o
Yes, my comments are confusing. The checks for sidecars use 
kMaxTotalSidecarBytes. However, several of the checks are to verify that 
arguments don't overflow. For example, SerializeMessage() takes additional_size 
as an int32_t, but the caller is using an int64_t local variable, so it's a bit 
different from the sidecar limits. I do something similar for the sidecar 
offsets, which are limited to uint32_t. I changed the comments to clarify this 
a bit. I also changed rpc-test.cc to use kMaxTotalSidecarBytes rather than 
INT_MAX.



--
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: 1
Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Comment-Date: Tue, 13 Mar 2018 20:05:45 +0000
Gerrit-HasComments: Yes

Reply via email to