Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
408633002:
Fix error handling for message parse errors that occur during connection setup. (Closed)
DescriptionFix error handling for message parse errors that occur during connection setup.
BUG=394940
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285769
Patch Set 1 #Patch Set 2 : Allow read event loop to close socket on error. #
Total comments: 17
Patch Set 3 : Addressed feedback, changed some DCHECKs to CHECKs #Patch Set 4 : Restored previous impl for hopping from read loop to connect loop. #Patch Set 5 : Misc. changes #
Total comments: 20
Patch Set 6 : Responded to review feedback #Patch Set 7 : Removed NOTREACHED #
Total comments: 8
Patch Set 8 : Removed superfluous size check #
Total comments: 15
Patch Set 9 : Changed some sanity checks back to CHECKs. #
Total comments: 2
Patch Set 10 : CHECK_NE => CHECK_GT #Patch Set 11 : Merging in changes #
Messages
Total messages: 43 (0 generated)
Thanks for taking a stab at this. Writing a unit tests that fails without the patch would be a really good exercise to make sure the patch is solid. If test cases can be added that cover both the messages the APK sent to us, as well as the specific code paths that were modified that would be ideal. The existing test has a framework (sort of) to cover both network operations that complete synchronously and asynchronously. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:161: return AuthenticateChallengeReply(*challenge_reply_, peer_cert_); Thank you for fixing this, in my email reply I forgot scoped_ptr overloads * and does the right thing: CHECK-fails on NULL dereference. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:554: DCHECK_LE(num_bytes_to_read, MessageHeader::header_size()); We should audit the use of DCHECKs in the read and connect loop and ensure that they shouldn't be converted to CHECKs. It would be better to CHECK-fail the browser than leave a potental buffer overflow. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:621: read_state_ = READ_STATE_ERROR; This is another place we need to abort the read loop. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:628: read_state_ = READ_STATE_ERROR; Ditto https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:638: // If inside connection flow, then get back to connect loop. I don't fully understand this piece of logic. A read error should immediately abort the connection attempt and not restart the connect loop. I think this is trying to guarantee that we don't invoke the connect callback (to signal the error) from a different callback passed to net::socket. However this is indirect and I'm not really convinced it's correct (what's the invariant on connect_state_ here)? At the least, make sure that there is a test case that a read error during connect does the right thing, and possibly add a CHECK-guard to guarantee that the connect loop exits on a read error. When Munjal is back, we can maybe think about if there is a cleaner way to handle the control flow - I believe he has more context for why we are posting tasks to hop between loops.
Also, before committing, suggest having rsleevi@ have a look, as his feedback largely determined the current shape of the code.
https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:161: return AuthenticateChallengeReply(*challenge_reply_, peer_cert_); On 2014/07/21 17:38:52, mark a. foltz wrote: > Thank you for fixing this, in my email reply I forgot scoped_ptr overloads * and > does the right thing: CHECK-fails on NULL dereference. Acknowledged. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:554: DCHECK_LE(num_bytes_to_read, MessageHeader::header_size()); On 2014/07/21 17:38:52, mark a. foltz wrote: > We should audit the use of DCHECKs in the read and connect loop and ensure that > they shouldn't be converted to CHECKs. It would be better to CHECK-fail the > browser than leave a potental buffer overflow. Done here and elsewhere in places that deal with sizes and offsets. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:621: read_state_ = READ_STATE_ERROR; On 2014/07/21 17:38:52, mark a. foltz wrote: > This is another place we need to abort the read loop. Already done - setting read_state_ to READ_STATE_ERROR has the effect of finishing the read event loop (indirectly: DoReadError leaves the read state as READ_STATE_NONE which is an exit condition for the do-while loop). https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:628: read_state_ = READ_STATE_ERROR; On 2014/07/21 17:38:52, mark a. foltz wrote: > Ditto Acknowledged. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:638: // If inside connection flow, then get back to connect loop. On 2014/07/21 17:38:52, mark a. foltz wrote: > I don't fully understand this piece of logic. A read error should immediately > abort the connection attempt and not restart the connect loop. I think this is > trying to guarantee that we don't invoke the connect callback (to signal the > error) from a different callback passed to net::socket. However this is > indirect and I'm not really convinced it's correct (what's the invariant on > connect_state_ here)? > > At the least, make sure that there is a test case that a read error during > connect does the right thing, and possibly add a CHECK-guard to guarantee that > the connect loop exits on a read error. When Munjal is back, we can maybe think > about if there is a cleaner way to handle the control flow - I believe he has > more context for why we are posting tasks to hop between loops. > Changed the flow to call the completion callback directly instead of via the messageloop. PTAL. Given the simplistic nature of the revised function (<any error>=><ERR_FAILED>) we could eliminate this node from the state machine entirely...
https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:638: // If inside connection flow, then get back to connect loop. On 2014/07/21 20:48:58, kmarshall wrote: > On 2014/07/21 17:38:52, mark a. foltz wrote: > > I don't fully understand this piece of logic. A read error should immediately > > abort the connection attempt and not restart the connect loop. I think this > is > > trying to guarantee that we don't invoke the connect callback (to signal the > > error) from a different callback passed to net::socket. However this is > > indirect and I'm not really convinced it's correct (what's the invariant on > > connect_state_ here)? > > > > At the least, make sure that there is a test case that a read error during > > connect does the right thing, and possibly add a CHECK-guard to guarantee that > > the connect loop exits on a read error. When Munjal is back, we can maybe > think > > about if there is a cleaner way to handle the control flow - I believe he has > > more context for why we are posting tasks to hop between loops. > > > > Changed the flow to call the completion callback directly instead of via the > messageloop. PTAL. Given the simplistic nature of the revised function (<any > error>=><ERR_FAILED>) we could eliminate this node from the state machine > entirely... I'm not 100% comfortable changing the control flow in this way without review by Munjal. The issue is that the completion callback may delete the CastSocket on the API side, and we don't want to do that inside a callback on the net:: side. Let's do that cleanup in a separate patch.
Note: I'm seeing segfaults when I close the browser after a Cast session on a release build, but they occur on a pristine client as well, so I don't think my change is responsible. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:638: // If inside connection flow, then get back to connect loop. On 2014/07/21 21:23:00, mark a. foltz wrote: > On 2014/07/21 20:48:58, kmarshall wrote: > > On 2014/07/21 17:38:52, mark a. foltz wrote: > > > I don't fully understand this piece of logic. A read error should > immediately > > > abort the connection attempt and not restart the connect loop. I think this > > is > > > trying to guarantee that we don't invoke the connect callback (to signal the > > > error) from a different callback passed to net::socket. However this is > > > indirect and I'm not really convinced it's correct (what's the invariant on > > > connect_state_ here)? > > > > > > At the least, make sure that there is a test case that a read error during > > > connect does the right thing, and possibly add a CHECK-guard to guarantee > that > > > the connect loop exits on a read error. When Munjal is back, we can maybe > > think > > > about if there is a cleaner way to handle the control flow - I believe he > has > > > more context for why we are posting tasks to hop between loops. > > > > > > > Changed the flow to call the completion callback directly instead of via the > > messageloop. PTAL. Given the simplistic nature of the revised function (<any > > error>=><ERR_FAILED>) we could eliminate this node from the state machine > > entirely... > > I'm not 100% comfortable changing the control flow in this way without review by > Munjal. The issue is that the completion callback may delete the CastSocket on > the API side, and we don't want to do that inside a callback on the net:: side. > > Let's do that cleanup in a separate patch. Done. OK, back to the previous method.
LGTM; apologies for the delay, and thanks for being patient with your trio of picky reviewers. ;) https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (left): https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:637: // does not try to report error also. This comment implies that if we return something other than net::OK then we're going to get a read-failure notification sent to the caller, even though we haven't yet informed them of the connection succeeding, i.e. when they won't be expecting one? https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:554: DCHECK_LE(num_bytes_to_read, MessageHeader::header_size()); On 2014/07/21 20:48:58, kmarshall wrote: > On 2014/07/21 17:38:52, mark a. foltz wrote: > > We should audit the use of DCHECKs in the read and connect loop and ensure > that > > they shouldn't be converted to CHECKs. It would be better to CHECK-fail the > > browser than leave a potental buffer overflow. > Done here and elsewhere in places that deal with sizes and offsets. Rather than [D]CHECK(), we could do the following: if (unexpected_condition) { NOTREACHED(); CloseWithError(GENERIC_FAILURE); ... return or whatever ... } This gets us graceful handling of failures in Release builds, but logged, and crashes in Debug builds. Downside is no visibility via go/crash into these issues occurring in the wild. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:594: return net::ERR_FAILED; IIUC this change will mean we get read error notifications for CastChannels that have neither succeeded nor failed connecting yet, which probably isn't what we want? https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:535: // which is an exit criteria for the finite state machine. This comment is really stipulating an invariant of DoReadError()'s behaviour, that read_state_ == READ_STATE_NONE after it has executed - so why not DCHECK_EQ(...) for that instead of the comment, to document the requirement? https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:545: // Handle errors that might have occurred. nit: This comment is superfluous. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:549: // will be sent on the connect cb instead.) nit: Suggest: "Read errors during the connection handshake should notify the caller via the connect callback, rather than the message event delegate." and update the else clause comment similarly. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:640: return net::ERR_INVALID_RESPONSE; Is it a problem that we're no longer clearing |current_message_| in case of error?
https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:571: CHECK_LE(num_bytes_to_read, MessageHeader::max_message_size()); BUG? It seems to me that the attacker has full control over the message size in a valid header, in which case, they can construct a message header that indicates a larger max_message_size If that is the case, you shouldn't CHECK(). That is, you should never CHECK() over 'remote' data, as that's a browser DOS. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:642: delegate_->OnMessage(this, message_info); Is the delegate_ allowed to delete this? If so, line 643 is inherently dangerous (really, we shouldn't be calling into the Delegate here, or in DoReadLoop, but waiting until the pc is in the 'outermost' layer of CastSocket) https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:648: VLOG_WITH_CONNECTION(1) << "DoReadError: " << result; Spammy. VLOG(2) if you have to. But really, you shouldn't. Long term, wire up the NetLog to Cast to aid in debugging. We discourage console spam for this. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:669: current_message_size_); SECURITY BUG: Crash on hostile input. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:719: CHECK(size > 0); CHECK_LT CHECK_GT SECURITY_BUG? Crash on hostile input?
https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:571: CHECK_LE(num_bytes_to_read, MessageHeader::max_message_size()); On 2014/07/22 22:52:43, Ryan Sleevi wrote: > BUG? It seems to me that the attacker has full control over the message size in > a valid header, in which case, they can construct a message header that > indicates a larger max_message_size > > If that is the case, you shouldn't CHECK(). That is, you should never CHECK() > over 'remote' data, as that's a browser DOS. Exactly; see my suggestion re the NOTREACHED() + failure result pattern.
On 2014/07/23 05:22:01, Wez wrote: > https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... > File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): > > https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... > chrome/browser/extensions/api/cast_channel/cast_socket.cc:571: > CHECK_LE(num_bytes_to_read, MessageHeader::max_message_size()); > On 2014/07/22 22:52:43, Ryan Sleevi wrote: > > BUG? It seems to me that the attacker has full control over the message size > in > > a valid header, in which case, they can construct a message header that > > indicates a larger max_message_size > > > > If that is the case, you shouldn't CHECK(). That is, you should never CHECK() > > over 'remote' data, as that's a browser DOS. > > Exactly; see my suggestion re the NOTREACHED() + failure result pattern. Scratch that... NOTREACHED() is, of course, not appropriate here precisely because the input is controlled by the caller; we should be trapping these cases as basic protocol errors instead, as early as possible, i.e. as soon as we can possibly detect that the input was bad.
https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:554: DCHECK_LE(num_bytes_to_read, MessageHeader::header_size()); On 2014/07/22 22:33:09, Wez wrote: > On 2014/07/21 20:48:58, kmarshall wrote: > > On 2014/07/21 17:38:52, mark a. foltz wrote: > > > We should audit the use of DCHECKs in the read and connect loop and ensure > > that > > > they shouldn't be converted to CHECKs. It would be better to CHECK-fail the > > > browser than leave a potental buffer overflow. > > Done here and elsewhere in places that deal with sizes and offsets. > > Rather than [D]CHECK(), we could do the following: > > if (unexpected_condition) { > NOTREACHED(); > CloseWithError(GENERIC_FAILURE); > ... return or whatever ... > } > > This gets us graceful handling of failures in Release builds, but logged, and > crashes in Debug builds. Downside is no visibility via go/crash into these > issues occurring in the wild. Done. https://codereview.chromium.org/408633002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:594: return net::ERR_FAILED; On 2014/07/22 22:33:09, Wez wrote: > IIUC this change will mean we get read error notifications for CastChannels that > have neither succeeded nor failed connecting yet, which probably isn't what we > want? This was removed. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:535: // which is an exit criteria for the finite state machine. On 2014/07/22 22:33:10, Wez wrote: > This comment is really stipulating an invariant of DoReadError()'s behaviour, > that read_state_ == READ_STATE_NONE after it has executed - so why not > DCHECK_EQ(...) for that instead of the comment, to document the requirement? :) Good idea. Done. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:545: // Handle errors that might have occurred. On 2014/07/22 22:33:09, Wez wrote: > nit: This comment is superfluous. Done. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:549: // will be sent on the connect cb instead.) On 2014/07/22 22:33:09, Wez wrote: > nit: Suggest: > "Read errors during the connection handshake should notify the caller via the > connect callback, rather than the message event delegate." > and update the else clause comment similarly. Done. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:571: CHECK_LE(num_bytes_to_read, MessageHeader::max_message_size()); On 2014/07/23 05:22:00, Wez wrote: > On 2014/07/22 22:52:43, Ryan Sleevi wrote: > > BUG? It seems to me that the attacker has full control over the message size > in > > a valid header, in which case, they can construct a message header that > > indicates a larger max_message_size > > > > If that is the case, you shouldn't CHECK(). That is, you should never CHECK() > > over 'remote' data, as that's a browser DOS. > > Exactly; see my suggestion re the NOTREACHED() + failure result pattern. Acknowledged. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:571: CHECK_LE(num_bytes_to_read, MessageHeader::max_message_size()); On 2014/07/22 22:52:43, Ryan Sleevi wrote: > BUG? It seems to me that the attacker has full control over the message size in > a valid header, in which case, they can construct a message header that > indicates a larger max_message_size > > If that is the case, you shouldn't CHECK(). That is, you should never CHECK() > over 'remote' data, as that's a browser DOS. Done. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:640: return net::ERR_INVALID_RESPONSE; On 2014/07/22 22:33:09, Wez wrote: > Is it a problem that we're no longer clearing |current_message_| in case of > error? Re-added that behavior. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:642: delegate_->OnMessage(this, message_info); On 2014/07/22 22:52:43, Ryan Sleevi wrote: > Is the delegate_ allowed to delete this? > > If so, line 643 is inherently dangerous (really, we shouldn't be calling into > the Delegate here, or in DoReadLoop, but waiting until the pc is in the > 'outermost' layer of CastSocket) No, the delegate is not allowed to delete this during OnMessage events. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:648: VLOG_WITH_CONNECTION(1) << "DoReadError: " << result; On 2014/07/22 22:52:43, Ryan Sleevi wrote: > Spammy. VLOG(2) if you have to. But really, you shouldn't. > > Long term, wire up the NetLog to Cast to aid in debugging. We discourage console > spam for this. Done. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:669: current_message_size_); On 2014/07/22 22:52:43, Ryan Sleevi wrote: > SECURITY BUG: Crash on hostile input. Done. https://codereview.chromium.org/408633002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:719: CHECK(size > 0); On 2014/07/22 22:52:43, Ryan Sleevi wrote: > CHECK_LT > CHECK_GT > > SECURITY_BUG? Crash on hostile input? Now returning a bool, which eventually yields an error if false is returned.
https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:545: // Connection not yet established. nit: No need for this; should be implicit from ready_state_ if, above. https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:729: if (size >= static_cast<size_t>(kuint32max) || Don't we already have a check against max_message_size() before we SetMessageSize()? Do we need a check here at all? And isn't this check a no-op on a 32-bit system anyway?
https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:729: if (size >= static_cast<size_t>(kuint32max) || On 2014/07/23 21:27:42, Wez wrote: > Don't we already have a check against max_message_size() before we > SetMessageSize()? Do we need a check here at all? And isn't this check a no-op > on a 32-bit system anyway? It would matter for 64-bit https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:733: message_size = static_cast<size_t>(size); Why is this static cast needed? It feels like both the check and the cast are artifacts of a different implementation?
https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:729: if (size >= static_cast<size_t>(kuint32max) || On 2014/07/23 22:31:13, Ryan Sleevi wrote: > On 2014/07/23 21:27:42, Wez wrote: > > Don't we already have a check against max_message_size() before we > > SetMessageSize()? Do we need a check here at all? And isn't this check a no-op > > on a 32-bit system anyway? > > It would matter for 64-bit Except that we already check against max_message_size(), at the calling site, which is 65535?
https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:545: // Connection not yet established. On 2014/07/23 21:27:42, Wez wrote: > nit: No need for this; should be implicit from ready_state_ if, above. Done. https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:729: if (size >= static_cast<size_t>(kuint32max) || On 2014/07/23 22:33:31, Wez wrote: > On 2014/07/23 22:31:13, Ryan Sleevi wrote: > > On 2014/07/23 21:27:42, Wez wrote: > > > Don't we already have a check against max_message_size() before we > > > SetMessageSize()? Do we need a check here at all? And isn't this check a > no-op > > > on a 32-bit system anyway? > > > > It would matter for 64-bit > > Except that we already check against max_message_size(), at the calling site, > which is 65535? True. Removed the check. Thanks. https://codereview.chromium.org/408633002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:733: message_size = static_cast<size_t>(size); On 2014/07/23 22:31:13, Ryan Sleevi wrote: > Why is this static cast needed? > > It feels like both the check and the cast are artifacts of a different > implementation? Done.
It's okay to convert to convert some of the DCHECKs to error states; but they were intended to enforce invariants in the code, not validate input. If we encounter these conditions in the field it would be helpful to have some idea why. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:564: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; This means that header_read_buffer_ was originally sized incorrectly, which is a bug. If that happens we will simply error out all messages, but not know why. Add a LOG(ERROR) explaining why we are closing the channel. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:572: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; This is also indicative of a bug, as current_message_size_ is constrained to be between 0 and max_message_size(), and body_read_buffer_->offset() is an offset into an IOBuffer of capacity max_message_size(). I would like a LOG(ERROR) explaining the condition. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:577: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; Ditto. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:729: return true; Why return a boolean if it's always true?
Sorry... I think Mark's right that these DCHECKs really were to catch logic errors, so they should be DCHECKs, if the code can tolerate the unexpected states in Release builds without ill effects, or CHECKs if their not being satisfied could lead to e.g. security problems. Apologies for the churn on this one, Kevin. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:564: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; On 2014/07/24 00:32:42, mark a. foltz wrote: > This means that header_read_buffer_ was originally sized incorrectly, which is a > bug. If that happens we will simply error out all messages, but not know why. > Add a LOG(ERROR) explaining why we are closing the channel. If this really indicates a logic error then it should be CHECK_LE(), as in an earlier revision, since it should never happen but if it does we mustn't continue processing the header. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:572: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; On 2014/07/24 00:32:42, mark a. foltz wrote: > This is also indicative of a bug, as current_message_size_ is constrained to be > between 0 and max_message_size(), and body_read_buffer_->offset() is an offset > into an IOBuffer of capacity max_message_size(). I would like a LOG(ERROR) > explaining the condition. Again, given that this situation in principle can never arise, let's CHECK(); that way we will get crash reports if the logic breaks. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:675: current_message_size_) { This also indicates a logic error; I can't see a way in which we can get here without this being true - just CHECK_EQ()? https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.h:126: bool SetMessageSize(size_t message_size); This no longer needs the Boolean return. :)
https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:564: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; On 2014/07/24 00:55:55, Wez wrote: > On 2014/07/24 00:32:42, mark a. foltz wrote: > > This means that header_read_buffer_ was originally sized incorrectly, which is > a > > bug. If that happens we will simply error out all messages, but not know why. > > > Add a LOG(ERROR) explaining why we are closing the channel. > > If this really indicates a logic error then it should be CHECK_LE(), as in an > earlier revision, since it should never happen but if it does we mustn't > continue processing the header. Done. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:564: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; On 2014/07/24 00:32:42, mark a. foltz wrote: > This means that header_read_buffer_ was originally sized incorrectly, which is a > bug. If that happens we will simply error out all messages, but not know why. > Add a LOG(ERROR) explaining why we are closing the channel. Acknowledged. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:572: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; On 2014/07/24 00:55:55, Wez wrote: > On 2014/07/24 00:32:42, mark a. foltz wrote: > > This is also indicative of a bug, as current_message_size_ is constrained to > be > > between 0 and max_message_size(), and body_read_buffer_->offset() is an offset > > into an IOBuffer of capacity max_message_size(). I would like a LOG(ERROR) > > explaining the condition. > > Again, given that this situation in principle can never arise, let's CHECK(); > that way we will get crash reports if the logic breaks. Done. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:572: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; On 2014/07/24 00:32:42, mark a. foltz wrote: > This is also indicative of a bug, as current_message_size_ is constrained to be > between 0 and max_message_size(), and body_read_buffer_->offset() is an offset > into an IOBuffer of capacity max_message_size(). I would like a LOG(ERROR) > explaining the condition. Acknowledged. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:577: error_state_ = CHANNEL_ERROR_INVALID_MESSAGE; On 2014/07/24 00:32:42, mark a. foltz wrote: > Ditto. Done. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:675: current_message_size_) { On 2014/07/24 00:55:55, Wez wrote: > This also indicates a logic error; I can't see a way in which we can get here > without this being true - just CHECK_EQ()? Done. https://codereview.chromium.org/408633002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:729: return true; On 2014/07/24 00:32:42, mark a. foltz wrote: > Why return a boolean if it's always true? Vestiges from validation in a previous patch. Thanks, fixed.
lgtm
The CQ bit was checked by kmarshall@chromium.org
The CQ bit was unchecked by kmarshall@chromium.org
The CQ bit was checked by kmarshall@chromium.org
lgtm https://codereview.chromium.org/408633002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:570: CHECK_NE(num_bytes_to_read, 0U); CHECK_GT?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/408633002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
https://codereview.chromium.org/408633002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/408633002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:570: CHECK_NE(num_bytes_to_read, 0U); On 2014/07/24 23:18:38, Wez wrote: > CHECK_GT? Done.
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/408633002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33390) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38427)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33398)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/408633002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33407) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38443)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33420)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmarshall@chromium.org/408633002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...) mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...) mac_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Message was sent while issue was closed.
Change committed as 285769
Message was sent while issue was closed.
On 2014/07/26 18:16:33, I haz the power (commit-bot) wrote: > Change committed as 285769 This causes a build failure with gcc 4.8.2. CL fixing the same here: https://codereview.chromium.org/421233002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
