You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2018/03/13 05:57:01 UTC
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9601
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that the
rpc_max_message_size is 32-bit integer, the receiver would
reject any message with sidecars this large anyway. This also
helps with the networking codepath, as any given sidecar will
have a size less than INT_MAX, so all Slices that interact
with Writev() are each less than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
14 files changed, 63 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/9601/1
--
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: newchange
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9601
to look at the new patch set (#6).
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that the
rpc_max_message_size is limited to INT_MAX at startup, the
receiver would reject any message with sidecars this large anyway.
This also helps with the networking codepath, as any given
sidecar will have a size less than INT_MAX, so all Slices
that interact with Writev() are each less than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX bytes.
- Added a test mode to TestRpcSidecarLimits() where it
overrides rpc_max_message_size and sends the maximal
message. This verifies that the client send codepath
can handle the maximal message.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_sidecar.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
16 files changed, 141 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/9601/6
--
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: newpatchset
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161: // SerializeMessage() takes the additional_size as an int32_t. Protect against
: // any overflow.
: CHECK_LE(sidecar_byte_size_, std::numeric_limits<int32_t>::max());
> If additional_size is less than INT_MAX in practice, is it possible to chan
That makes sense to me. I'm working on that change.
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:18:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 16 Mar 2018 21:29:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Mar 2018 20:05:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9601
to look at the new patch set (#2).
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that the
rpc_max_message_size is 32-bit integer, the receiver would
reject any message with sidecars this large anyway. This also
helps with the networking codepath, as any given sidecar will
have a size less than INT_MAX, so all Slices that interact
with Writev() are each less than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
14 files changed, 76 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/9601/2
--
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: newpatchset
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9601
to look at the new patch set (#3).
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that the
rpc_max_message_size is 32-bit integer, the receiver would
reject any message with sidecars this large anyway. This also
helps with the networking codepath, as any given sidecar will
have a size less than INT_MAX, so all Slices that interact
with Writev() are each less than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_sidecar.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
16 files changed, 80 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/9601/3
--
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: newpatchset
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG@20
PS8, Line 20: rpc_max_message_size
> Seems clearer to say FLAGS_rpc_max_message_size
Done
http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG@21
PS8, Line 21: any message with sidecars this large
> regardless of whether there is any sidecar, "any message this large" is rej
Changed the language to "any message this large".
The sidecars are the primary component. Protobuf is unlikely to be anywhere near INT_MAX, but if it is, that message would be rejected as well.
http://gerrit.cloudera.org:8080/#/c/9601/8/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/9601/8/src/kudu/rpc/rpc-test.cc@709
PS8, Line 709: string max_string;
: max_string.resize(TransferLimits::kMaxTotalSidecarBytes, 'a');
> string max_string(TransferLimits::kMaxTotalSidecarBytes, 'a');
Done
clang-tidy has a rule that flags large string initialization, but it only works on string constructors. I got around that for my INT_MAX string by doing a resize.
I was curious whether clang-tidy was smart enough to flag this:
string max_string(TransferLimits::kMaxTotalSidecarBytes, 'a');
And apparently, it isn't. So, I switched this to what you suggested.
--
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: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 21 Mar 2018 21:42:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc@724
PS7, Line 724: CHECK
> this can be an ASSERT_FALSE
Done
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc@761
PS7, Line 761: CHECK_OK
> this can be ASSERT_OK right?
Done
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc@783
PS7, Line 783: // Reset rpc_max_message_size to its default value
> This is done automatically by our base test class using google::FlagSaver s
Ok, that is good to know. Removed.
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc_sidecar.cc
File src/kudu/rpc/rpc_sidecar.cc:
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc_sidecar.cc@80
PS7, Line 80: %d
> this should be $1
Done
--
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: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 21 Mar 2018 00:09:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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');
> The receiver protects itself by closing the connection when it sees a messa
I'm taking another pass. At the point where we are in serialization::ParseMessage(), the sender has already sent the entire message, so it tests the send codepath. Rewriting a couple things to avoid using the CodedInputStream for the sidecars should be enough.
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 18 Mar 2018 16:25:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9601 )
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that
FLAGS_rpc_max_message_size is limited to INT_MAX at startup,
the receiver would reject any message this large anyway.
This also helps with the networking codepath, as any given
sidecar will have a size less than INT_MAX, so every Slice
that interacts with Writev() is shorter than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX bytes.
- Added a test mode to TestRpcSidecarLimits() where it
overrides rpc_max_message_size and sends the maximal
message. This verifies that the client send codepath
can handle the maximal message.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Reviewed-on: http://gerrit.cloudera.org:8080/9601
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_sidecar.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
16 files changed, 138 insertions(+), 38 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 10
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho 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:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.h@253
PS3, Line 253: int64_t
I wonder if this should also be int32_t to match the equivalent in OutboundCall ?
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.cc
File src/kudu/rpc/inbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.cc@225
PS3, Line 225: outbound_sidecars_total_bytes_ += sidecar_bytes;
If you end up converting outbound_sidecars_total_bytes_ to int32_t as suggested elsewhere, you may wanna DCHECK_GE(outbound_sidecrs_total_bytes_, 0) here.
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/outbound_call.h@276
PS3, Line 276: // Total size in bytes of all sidecars in 'sidecars_'. Set in SetRequestPayload().
Add a comment that this should not exceed TransferLimits::kMaxTotalSidecarBytes
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_controller.h@275
PS3, Line 275: int64_t
Again, we seem a bit inconsistent on whether this should be 32-bit or 64-bit.
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 16 Mar 2018 22:44:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.h@253
PS3, Line 253: int64_t
> I wonder if this should also be int32_t to match the equivalent in Outbound
Changed to int32_t.
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.cc
File src/kudu/rpc/inbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.cc@225
PS3, Line 225: outbound_sidecars_total_bytes_ += sidecar_bytes;
> If you end up converting outbound_sidecars_total_bytes_ to int32_t as sugge
Done
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/outbound_call.h@276
PS3, Line 276: // Total size in bytes of all sidecars in 'sidecars_'. Set in SetRequestPayload().
> Add a comment that this should not exceed TransferLimits::kMaxTotalSidecarB
Added a comment.
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');
> I'm taking another pass. At the point where we are in serialization::ParseM
Added a test that transfers the maximal message. The transfer goes through, but the server immediately rejects it (in a way separate from the rpc_max_message_size).
rpc_max_message_size is now an int64_t, but it is limited to a maximum of INT_MAX at startup.
Note that this test case does not cause the nwritten arguments in Socket::Writev() or TlsSocket::Writev() to reach INT_MAX.
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:
http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_controller.h@275
PS3, Line 275: int64_t
> Again, we seem a bit inconsistent on whether this should be 32-bit or 64-bi
Changed to int32_t.
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 19 Mar 2018 18:37:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161: // SerializeMessage() takes the additional_size as an int32_t. Protect against
: // any overflow.
: CHECK_LE(sidecar_byte_size_, std::numeric_limits<int32_t>::max());
> I still find the type mismatch a bit confusing.
In the abstract, using int32_t can make the compiler detect problems. Compilers can easily tell when a large type is being coerced to a smaller type. With appropriate warnings, a caller using an invalid conversion is a compile issue that forces them to think about the appropriate data types. With an int64_t argument, the only checks would happen at runtime and only if you have a test case that hits it.
Now, in practice, the warnings either aren't enabled or didn't detect this. We already know that callers aren't going to exceed INT_MAX anyway, so I don't have a very strong preference here.
Do Kudu reviewers have a strong preference?
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 22:49:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161: // SerializeMessage() takes the additional_size as an int32_t. Protect against
: // any overflow.
: CHECK_LE(sidecar_byte_size_, std::numeric_limits<int32_t>::max());
> Why not just replace additional_size with uint32_t and avoid this CHECK() a
Maybe these should be DCHECKs. We know that they can't be triggered, because sidecars are limited to INT_MAX.
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/serialization.cc
File src/kudu/rpc/serialization.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/serialization.cc@57
PS2, Line 57: int additional_size
> Why is this not uint32_t ?
I don't have strong feelings about this.
With the limits on sidecars, we don't need to pass in more than INT_MAX for additional_size. rpc_max_message_size is an int32, so a receiver cannot be configured to accept a message bigger than INT_MAX.
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/util/net/socket.cc@451
PS2, Line 451: int res
> Just wondering if there is any guarantee the return value will not overflow
It sounds like Linux limits the total size of a iovec to less than INT_MAX:
https://www.systutorials.com/linux-kernels/17760/net-limit-socket-i-o-iovec-total-length-to-int_max-linux-2-6-34-9/
It might be good form to make this a ssize_t (return type of sendmsg).
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 19:49:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 6:
Rebase to get related change to TestRpcSidecarLimits.
--
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: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 20 Mar 2018 17:59:22 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9601
to look at the new patch set (#8).
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that the
rpc_max_message_size is limited to INT_MAX at startup, the
receiver would reject any message with sidecars this large anyway.
This also helps with the networking codepath, as any given
sidecar will have a size less than INT_MAX, so all Slices
that interact with Writev() are each less than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX bytes.
- Added a test mode to TestRpcSidecarLimits() where it
overrides rpc_max_message_size and sends the maximal
message. This verifies that the client send codepath
can handle the maximal message.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_sidecar.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
16 files changed, 139 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/9601/8
--
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: newpatchset
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho 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 8: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG@20
PS8, Line 20: rpc_max_message_size
Seems clearer to say FLAGS_rpc_max_message_size
http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG@21
PS8, Line 21: any message with sidecars this large
regardless of whether there is any sidecar, "any message this large" is rejected, right ? Of course, it's unlikely for such large message to not have sidecar.
http://gerrit.cloudera.org:8080/#/c/9601/8/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/9601/8/src/kudu/rpc/rpc-test.cc@709
PS8, Line 709: string max_string;
: max_string.resize(TransferLimits::kMaxTotalSidecarBytes, 'a');
string max_string(TransferLimits::kMaxTotalSidecarBytes, 'a');
--
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: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 21 Mar 2018 20:40:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc@724
PS7, Line 724: CHECK
this can be an ASSERT_FALSE
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc@761
PS7, Line 761: CHECK_OK
this can be ASSERT_OK right?
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc-test.cc@783
PS7, Line 783: // Reset rpc_max_message_size to its default value
This is done automatically by our base test class using google::FlagSaver so not necessary here
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc_sidecar.cc
File src/kudu/rpc/rpc_sidecar.cc:
http://gerrit.cloudera.org:8080/#/c/9601/7/src/kudu/rpc/rpc_sidecar.cc@80
PS7, Line 80: %d
this should be $1
--
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: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 20 Mar 2018 23:52:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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:
(1 comment)
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');
> I will look into what test could exist. The awkward thing is that maxing ou
I'm afraid about fixing hypotehtical issues with a non-hypothetical test. How do we know we fixed it? Aren't we getting some false confidence, then, if we haven't proven it?
What about making the flag an int64 but adding a validator that it can't be set more than 2GB or whatever you think is a reasonable max for Impala? In the tests, then, by setting the flag variable manually you can override the validator.
Also, shouldn't the maximum match up between the client side and server? What you said above is a bit concerning - why is the client sending a message that the server will reject?
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 16 Mar 2018 21:43:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 5:
TSAN introduces memory overhead. It looks like the OS memory sniper is killing rpc-test when it runs the new transfer component of the test. On my machine with 32GB, the test runs to completion, but it uses 11GB at one point. This upload disables the transfer part of TestRpcSidecarLimits on TSAN (the existing parts of TestRpcSidecarLimits still run).
--
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: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 19 Mar 2018 21:21:02 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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 'size' is often used to mean the length of a vector rather than the number of 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_offset to int64 above so the assertion is less likely to be dealing with an overflow if something goes wrong?
http://gerrit.cloudera.org:8080/#/c/9601/1/src/kudu/rpc/inbound_call.cc@221
PS1, Line 221: exceeds limit
would exceed
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
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
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 of kMaxTotalSidecarSize
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Mar 2018 14:59:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 2:
I'm having trouble reproducing the ITClientStress result that the Kudu Jenkins saw. It may not be related.
I'm running Impala's tests with this change.
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 18:50:13 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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');
> I'm afraid about fixing hypotehtical issues with a non-hypothetical test. H
The receiver protects itself by closing the connection when it sees a message larger than rpc_max_message_size. The question is whether the sender can ever keeping sending and overflow its own variables, particularly in TlsSocket::Writev(). That is why this is hypothetical. I don't think it is truly possible or at least it is extremely hard to hit, but changing some variables to a larger size can't really hurt.
I played around with writing a test for it. The problem is that testing the sender code requires that the receiver keep the connection open, so the receiver needs to support messages larger than INT_MAX. It doesn't today. If it ignores the rpc_max_message_size, then runs into the fact that Socket::Recv() doesn't support more than INT_MAX at a time. When you get past that, serialization::ParseMessage() also doesn't support more than INT_MAX, because CodedInputStream takes an int for the buffer size.
Long story short, this is a tough test to write.
If the client knew the header and protobuf sizes in the right places, it could make the client and server match up. It doesn't at the moment.
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 17 Mar 2018 00:36:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho 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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161: // SerializeMessage() takes the additional_size as an int32_t. Protect against
: // any overflow.
: CHECK_LE(sidecar_byte_size_, std::numeric_limits<int32_t>::max());
> In the abstract, using int32_t can make the compiler detect problems. Compi
If additional_size is less than INT_MAX in practice, is it possible to change sidecar_byte_size_ to int32_t instead ? In which case, there is no mismatch.
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 23:22:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9601
to look at the new patch set (#4).
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that the
rpc_max_message_size is limited to INT_MAX at startup, the
receiver would reject any message with sidecars this large anyway.
This also helps with the networking codepath, as any given
sidecar will have a size less than INT_MAX, so all Slices
that interact with Writev() are each less than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX bytes.
- Added a test mode to TestRpcSidecarLimits() where it
overrides rpc_max_message_size and sends the maximal
message. This verifies that the client send codepath
can handle the maximal message.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_sidecar.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
16 files changed, 138 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/9601/4
--
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: newpatchset
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho 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 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161: // SerializeMessage() takes the additional_size as an int32_t. Protect against
: // any overflow.
: CHECK_LE(sidecar_byte_size_, std::numeric_limits<int32_t>::max());
Why not just replace additional_size with uint32_t and avoid this CHECK() altogether ?
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/serialization.cc
File src/kudu/rpc/serialization.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/serialization.cc@57
PS2, Line 57: int additional_size
Why is this not uint32_t ?
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 18:56:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho 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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161: // SerializeMessage() takes the additional_size as an int32_t. Protect against
: // any overflow.
: CHECK_LE(sidecar_byte_size_, std::numeric_limits<int32_t>::max());
> Maybe these should be DCHECKs. We know that they can't be triggered, becaus
I still find the type mismatch a bit confusing.
Given the computation of total size in SerializeMessage() is using int64_t and sidecar_byte_size_ is also int64_t, I don't see why we just standardize on just switching the type of additional_size to int64_t and skip the cast inside SerialzieMessage() ? What's the benefit of keeping it as int32_t given 64-bit machines have 64-bit registers anyway.
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 20:22:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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 9: Verified+1 Code-Review+2
--
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: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 21 Mar 2018 22:10:52 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Dan Burkert, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9601
to look at the new patch set (#9).
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
Inspection of the code revealed some other local variables
that could overflow with large messages. This patch takes
two approaches to eliminate the issues.
First, it limits the total size of the messages by limiting
the total size of the sidecars to INT_MAX. The total size
of the protobuf and header components of the message
should be considerably smaller, so limiting the sidecars
to INT_MAX eliminates messages that are larger than UINT_MAX.
This also means that the sidecar offsets, which are unsigned
32-bit integers, are also safe. Given that
FLAGS_rpc_max_message_size is limited to INT_MAX at startup,
the receiver would reject any message this large anyway.
This also helps with the networking codepath, as any given
sidecar will have a size less than INT_MAX, so every Slice
that interacts with Writev() is shorter than INT_MAX.
Second, even with sidecars limited to INT_MAX, the headers
and protobuf parts of the messages mean that certain messages
could still exceed INT_MAX. This patch changes some of the sockets
codepath to tolerate iovec's that reference more than INT_MAX
bytes total. Specifically, it changes Writev()'s nwritten bytes
to an int64_t for both TlsSocket and Socket. TlsSocket works
because it is sending each Slice individually. The first change
limited any given Slice to INT_MAX, so each individual Write()
should not be impacted. For Socket, Writev() uses sendmsg(). It
should do partial network sends to handle this case. Any Write()
call specifies its size with a 32-bit integer, and that will
not be impacted by this patch.
Testing:
- Modified TestRpcSidecarLimits() to verify that sidecars are
limited to INT_MAX bytes.
- Added a test mode to TestRpcSidecarLimits() where it
overrides rpc_max_message_size and sends the maximal
message. This verifies that the client send codepath
can handle the maximal message.
Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
---
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_sidecar.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
16 files changed, 138 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/9601/9
--
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: newpatchset
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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:
There are probably some comments that need to be updated. src/kudu/rpc/rpc_sidecar.h might need to note the limit on sidecar size.
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 13 Mar 2018 05:58:33 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161:
:
: Status OutboundCall::status() const {
> That makes sense to me. I'm working on that change.
Yea, we don't have the warnings enabled for the integer casts, but perhaps we should. The reason we didn't turn it on long ago is that it makes common loops like: for (int i = 0; i < foo.size(); i++) {} complain because vector::size is int64, etc. Given C++11 for-range loops, though, I think those are relatively less common (I count 134 currently).
We do have UBSAN enabled for our test builds, but being a dynamic check it relies on us actually exercising these corner cases.
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 15 Mar 2018 23:51:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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)
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 sidecar of the maximum, after configuring rpc_max_message_size to allow it? I think that woudl be a good check for overflow conditions, etc, given our use of UBSAN.
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 total size of the message (sidecars PLUS pb PLUS header?) Isn't 'buffer' here the entire message including everything, not just the sidecar portion?
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 16 Mar 2018 20:57:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has removed a vote on this change.
Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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:
The test failure on master_cert_authority-test seems unrelated. the IWYU issue is likely real. Let me look at the patch though
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Mar 2018 14:52:54 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 7:
Testing locally, I do not see the errors from the gerrit run. They look unrelated to the code change.
--
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: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 20 Mar 2018 20:04:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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:
This is needed for Impala's KRPC work. Is there a Kudu reviewer that can review/approve this?
--
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 <jo...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 16 Mar 2018 18:11:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho 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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/util/net/socket.cc@451
PS2, Line 451: int res
Just wondering if there is any guarantee the return value will not overflow an int ?
--
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: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 19:05:01 +0000
Gerrit-HasComments: Yes