Clean up type usage and fix MSVC "truncated value" warnings in net/quic/.

Clean up type usage and fix MSVC "truncated value" warnings in net/quic/.

The existing code is very inconsistent about whether it uses typedefs like
QuickByteCount and QuicPacketCount or whether it instead uses fundamental types
like uint32, uint64, and size_t.  This is not only more confusing to read, in
some cases it leads to MSVC warning about "possibly truncated value" when one
type is implicitly converted to another, shorter one.

This attempts to fix at least some of these sorts of problems by using the
typedefed types more widely when possible, or else at least the same basic types other places are already using.  It also fixes various warnings by including explicit casts (sometimes with legality checks).

BUG=81439
TEST=none

Committed: https://crrev.com/a9f22d4e065a52096ba58961d56a5e86a5a42675
Cr-Commit-Position: refs/heads/master@{#306276}

[Peter Kasting, 2014-11-27 01:58:36]

pkasting@chromium.org changed reviewers:
+ rtenneti@chromium.org

[Peter Kasting, 2014-11-27 01:58:37]

This patch isn't nearly a complete fix for the described issues, but I wanted to
stop before I went completely over the edge.

https://codereview.chromium.org/761863002/diff/20001/net/quic/crypto/crypto_u...
File net/quic/crypto/crypto_utils.cc (right):

https://codereview.chromium.org/761863002/diff/20001/net/quic/crypto/crypto_u...
net/quic/crypto/crypto_utils.cc:39: size_t bytes_written = 4;
It's arguable whether this code or the old code is better, but since the code is
hardcoded to write 4 bytes (as opposed to e.g. "while (x != 0) x >>= 8"), better
to say that.

https://codereview.chromium.org/761863002/diff/20001/net/quic/crypto/p256_key...
File net/quic/crypto/p256_key_exchange_nss.cc (right):

https://codereview.chromium.org/761863002/diff/20001/net/quic/crypto/p256_key...
net/quic/crypto/p256_key_exchange_nss.cc:135: uint16 private_key_size =
base::checked_cast<uint16>(private_key.size());
The old code sized |result| according to the casted value here but memcopied
based on the un-casted value.  That seems like memory corruption, and maybe a
security bug, to me.  Better to force a crash here in that case.

https://codereview.chromium.org/761863002/diff/20001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/761863002/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:254:
DCHECK_LT(curve25519_public_value.size(), (1U << 24));
I assumed these values couldn't be > 24 bits, but I would be happy to instead
use one of the safe_conversions.h checks to enforce that unconditionally if
desirable.

[chromium-reviews, 2014-11-27 02:11:49]

Hi Peter,
  Many many thanks for doing this cleanup. I will port this CL into our
internal tree and test it (including testing QUIC in chrome). Will do a
detailed review asap.

-raman

On Wed, Nov 26, 2014 at 5:58 PM, <pkasting@chromium.org> wrote:

> Reviewers: ramant,
>
> Message:
> This patch isn't nearly a complete fix for the described issues, but I
> wanted to
> stop before I went completely over the edge.
>
>
> https://codereview.chromium.org/761863002/diff/20001/net/
> quic/crypto/crypto_utils.cc
> File net/quic/crypto/crypto_utils.cc (right):
>
> https://codereview.chromium.org/761863002/diff/20001/net/
> quic/crypto/crypto_utils.cc#newcode39
> net/quic/crypto/crypto_utils.cc:39: size_t bytes_written = 4;
> It's arguable whether this code or the old code is better, but since the
> code is hardcoded to write 4 bytes (as opposed to e.g. "while (x != 0) x
>
>> = 8"), better to say that.
>>>
>>
> https://codereview.chromium.org/761863002/diff/20001/net/
> quic/crypto/p256_key_exchange_nss.cc
> File net/quic/crypto/p256_key_exchange_nss.cc (right):
>
> https://codereview.chromium.org/761863002/diff/20001/net/
> quic/crypto/p256_key_exchange_nss.cc#newcode135
> net/quic/crypto/p256_key_exchange_nss.cc:135: uint16 private_key_size =
> base::checked_cast<uint16>(private_key.size());
> The old code sized |result| according to the casted value here but
> memcopied based on the un-casted value.  That seems like memory
> corruption, and maybe a security bug, to me.  Better to force a crash
> here in that case.
>
> https://codereview.chromium.org/761863002/diff/20001/net/
> quic/crypto/quic_crypto_server_config.cc
> File net/quic/crypto/quic_crypto_server_config.cc (right):
>
> https://codereview.chromium.org/761863002/diff/20001/net/
> quic/crypto/quic_crypto_server_config.cc#newcode254
> net/quic/crypto/quic_crypto_server_config.cc:254:
> DCHECK_LT(curve25519_public_value.size(), (1U << 24));
> I assumed these values couldn't be > 24 bits, but I would be happy to
> instead use one of the safe_conversions.h checks to enforce that
> unconditionally if desirable.
>
> Description:
> Clean up type usage and fix MSVC "truncated value" warnings in net/quic/.
>
> The existing code is very inconsistent about whether it uses typedefs like
> QuickByteCount and QuicPacketCount or whether it instead uses fundamental
> types
> like uint32, uint64, and size_t.  This is not only more confusing to read,
> in
> some cases it leads to MSVC warning about "possibly truncated value" when
> one
> type is implicitly converted to another, shorter one.
>
> This attempts to fix at least some of these sorts of problems by using the
> typedefed types more widely when possible, or else at least the same basic
> types
> other places are already using.  It also fixes various warnings by
> including
> explicit casts (sometimes with legality checks).
>
> BUG=81439
> TEST=none
>
> Please review this at https://codereview.chromium.org/761863002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+269, -224 lines):
>   M net/http/http_server_properties.h
>   M net/quic/congestion_control/cube_root.cc
>   M net/quic/congestion_control/cubic.h
>   M net/quic/congestion_control/cubic.cc
>   M net/quic/congestion_control/cubic_test.cc
>   M net/quic/congestion_control/rtt_stats.cc
>   M net/quic/congestion_control/tcp_cubic_sender.h
>   M net/quic/congestion_control/tcp_loss_algorithm.h
>   M net/quic/crypto/cert_compressor.cc
>   M net/quic/crypto/crypto_framer.cc
>   M net/quic/crypto/crypto_utils.cc
>   M net/quic/crypto/p256_key_exchange_nss.cc
>   M net/quic/crypto/quic_crypto_server_config.cc
>   M net/quic/crypto/source_address_token.h
>   M net/quic/crypto/source_address_token.cc
>   M net/quic/crypto/strike_register.cc
>   M net/quic/quic_bandwidth.cc
>   M net/quic/quic_client_session.cc
>   M net/quic/quic_config.h
>   M net/quic/quic_config.cc
>   M net/quic/quic_connection.h
>   M net/quic/quic_connection.cc
>   M net/quic/quic_connection_logger.h
>   M net/quic/quic_connection_logger.cc
>   M net/quic/quic_connection_stats.h
>   M net/quic/quic_connection_stats.cc
>   M net/quic/quic_connection_test.cc
>   M net/quic/quic_crypto_client_stream.cc
>   M net/quic/quic_data_writer.cc
>   M net/quic/quic_fec_group.cc
>   M net/quic/quic_flow_controller.h
>   M net/quic/quic_flow_controller.cc
>   M net/quic/quic_flow_controller_test.cc
>   M net/quic/quic_framer.cc
>   M net/quic/quic_protocol.h
>   M net/quic/quic_received_packet_manager.cc
>   M net/quic/quic_sent_packet_manager.cc
>   M net/quic/quic_server.cc
>   M net/quic/quic_session.h
>   M net/quic/quic_session.cc
>   M net/quic/quic_stream_factory.cc
>   M net/quic/quic_stream_sequencer.cc
>   M net/quic/quic_time.cc
>   M net/quic/quic_unacked_packet_map.h
>   M net/quic/quic_unacked_packet_map.cc
>   M net/quic/quic_utils.cc
>   M net/quic/reliable_quic_stream.h
>   M net/quic/reliable_quic_stream.cc
>   M net/quic/test_tools/quic_flow_controller_peer.h
>   M net/quic/test_tools/quic_flow_controller_peer.cc
>   M net/quic/test_tools/quic_sent_packet_manager_peer.h
>   M net/quic/test_tools/quic_sent_packet_manager_peer.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-11-27 02:13:34]

On 2014/11/27 02:11:49, chromium-reviews wrote:
> Hi Peter,
>   Many many thanks for doing this cleanup. I will port this CL into our
> internal tree and test it (including testing QUIC in chrome). Will do a
> detailed review asap.

You'll want to wait to test it until I get the trybots green, of course :).

[chromium-reviews, 2014-11-27 02:17:55]

Will test after the trybots tun green.

On Wed, Nov 26, 2014 at 6:13 PM, <pkasting@chromium.org> wrote:

> On 2014/11/27 02:11:49, chromium-reviews wrote:
>
>> Hi Peter,
>>    Many many thanks for doing this cleanup. I will port this CL into our
>> internal tree and test it (including testing QUIC in chrome). Will do a
>> detailed review asap.
>>
>
> You'll want to wait to test it until I get the trybots green, of course :).
>
> https://codereview.chromium.org/761863002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ramant (doing other things), 2014-11-30 21:38:18]

LGTM.

[Peter Kasting, 2014-12-01 22:39:28]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-12-01 22:40:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/761863002/80001

[commit-bot: I haz the power, 2014-12-01 22:43:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-01 22:44:38]

Patchset 5 (id:??) landed as
https://crrev.com/a9f22d4e065a52096ba58961d56a5e86a5a42675
Cr-Commit-Position: refs/heads/master@{#306276}