Change CryptoHandshakeMessage::GetTaglist to tag a QuicTagVector*

Change CryptoHandshakeMessage::GetTaglist to tag a QuicTagVector*
instead of a QuicTag** to avoid unaligned read problems on arm.

Merge internal change: 157281990

BUG=726137
Review-Url: https://codereview.chromium.org/2907743003
Cr-Commit-Position: refs/heads/master@{#475255}
Committed: https://chromium.googlesource.com/chromium/src/+/ebba10fb085b8b01e2faa64523bed3e5373f8b5e

[Ryan Hamilton, 2017-05-27 00:23:10]

rch@chromium.org changed reviewers:
+ jri@chromium.org

[Ryan Hamilton, 2017-05-27 00:23:11]

jri: PTAL
thakis: FYI

[Ryan Hamilton, 2017-05-27 00:23:17]

The CQ bit was checked by rch@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 00:23:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-27 00:33:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 00:33:45]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Nico, 2017-05-27 00:34:31]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-05-27 00:34:32]

Lgtm, thanks!

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/crypto/quic_c...
File net/quic/core/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/crypto/quic_c...
net/quic/core/crypto/quic_crypto_server_config.cc:1614: if
(msg->GetTaglist(kAEAD, &(config->aead)) != QUIC_NO_ERROR) {
Nit: no parens

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/crypto/quic_c...
net/quic/core/crypto/quic_crypto_server_config.cc:1626: if ((err =
msg->GetTaglist(kTBKP, &(config->tb_key_params))) !=
Ditto

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/quic_config.cc
File net/quic/core/quic_config.cc (left):

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/quic_config.c...
net/quic/core/quic_config.cc:238: peer_hello.GetTaglist(tag_, &received_tags,
&received_tags_length);
Here, if this failed the 2nd time, the LHS would keep the tags from the first
time around
 Now it clears them all. Ok?

[Ryan Hamilton, 2017-05-27 00:39:54]

Thanks!

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/crypto/quic_c...
File net/quic/core/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/crypto/quic_c...
net/quic/core/crypto/quic_crypto_server_config.cc:1614: if
(msg->GetTaglist(kAEAD, &(config->aead)) != QUIC_NO_ERROR) {
On 2017/05/27 00:34:32, Nico wrote:
> Nit: no parens

Done.

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/crypto/quic_c...
net/quic/core/crypto/quic_crypto_server_config.cc:1626: if ((err =
msg->GetTaglist(kTBKP, &(config->tb_key_params))) !=
On 2017/05/27 00:34:32, Nico wrote:
> Ditto

Done.

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/quic_config.cc
File net/quic/core/quic_config.cc (left):

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/quic_config.c...
net/quic/core/quic_config.cc:238: peer_hello.GetTaglist(tag_, &received_tags,
&received_tags_length);
On 2017/05/27 00:34:32, Nico wrote:
> Here, if this failed the 2nd time, the LHS would keep the tags from the first
> time around
>  Now it clears them all. Ok?

Interesting point! Not a problem given how this is used, thankfully.

[Ryan Hamilton, 2017-05-27 00:47:12]

The CQ bit was checked by rch@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 00:47:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-05-27 00:51:32]

lgtm++

[commit-bot: I haz the power, 2017-05-27 00:56:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 00:56:42]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Ryan Hamilton, 2017-05-28 01:28:06]

Description was changed from

==========
Change CryptoHandshakeMessage::GetTaglist to tag a QuicTagVector*
instead of a QuicTag** to avoid unaligned read problems on arm.

BUG=726137
==========

to

==========
Change CryptoHandshakeMessage::GetTaglist to tag a QuicTagVector*
instead of a QuicTag** to avoid unaligned read problems on arm.

Merge internal change: 157281990

BUG=726137
==========

[Ryan Hamilton, 2017-05-28 01:29:19]

Thanks for the review! landing now... Go go gadget -Oz!

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/quic_config.cc
File net/quic/core/quic_config.cc (left):

https://codereview.chromium.org/2907743003/diff/1/net/quic/core/quic_config.c...
net/quic/core/quic_config.cc:238: peer_hello.GetTaglist(tag_, &received_tags,
&received_tags_length);
On 2017/05/27 00:39:54, Ryan Hamilton wrote:
> On 2017/05/27 00:34:32, Nico wrote:
> > Here, if this failed the 2nd time, the LHS would keep the tags from the
first
> > time around
> >  Now it clears them all. Ok?
> 
> Interesting point! Not a problem given how this is used, thankfully.

Interestingly, even though we don't make use of the behavior you described, we
do have a unit test of it, so I changed this to use the old behavior. Thanks for
spotting this!

[Nico, 2017-05-28 02:34:32]

The CQ bit was checked by thakis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-28 02:34:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-05-28 03:21:25]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2017-05-28 03:21:32]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-05-28 03:21:32]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2907743003/#ps40001
(title: "fix QuicConfig")

[commit-bot: I haz the power, 2017-05-28 03:21:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-28 04:24:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495941692059250,
"parent_rev": "b28281f7ab8040aeea063d0e9836704c8b1a454a", "commit_rev":
"ebba10fb085b8b01e2faa64523bed3e5373f8b5e"}

[commit-bot: I haz the power, 2017-05-28 04:24:21]

Description was changed from

==========
Change CryptoHandshakeMessage::GetTaglist to tag a QuicTagVector*
instead of a QuicTag** to avoid unaligned read problems on arm.

Merge internal change: 157281990

BUG=726137
==========

to

==========
Change CryptoHandshakeMessage::GetTaglist to tag a QuicTagVector*
instead of a QuicTag** to avoid unaligned read problems on arm.

Merge internal change: 157281990

BUG=726137

Review-Url: https://codereview.chromium.org/2907743003
Cr-Commit-Position: refs/heads/master@{#475255}
Committed:
https://chromium.googlesource.com/chromium/src/+/ebba10fb085b8b01e2faa64523be...
==========

[commit-bot: I haz the power, 2017-05-28 04:24:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ebba10fb085b8b01e2faa64523be...

[Jana, 2017-05-30 02:57:06]

lgtm

Late, but LGTM

[Nico, 2017-06-12 20:32:31]

https://codereview.chromium.org/2907743003/diff/40001/net/quic/core/crypto/cr...
File net/quic/core/crypto/crypto_handshake_message.cc (right):

https://codereview.chromium.org/2907743003/diff/40001/net/quic/core/crypto/cr...
net/quic/core/crypto/crypto_handshake_message.cc:103: memcpy(&tag,
it->second.data() + i * sizeof(tag), sizeof(tag));
A few short weeks later, I suddenly remembered this CL and that I forgot to
mention that you could've used
https://cs.chromium.org/chromium/src/base/bit_cast.h?type=cs&q=bit_cast+packa...
here if you can depend on base.

[Ryan Hamilton, 2017-06-12 20:48:41]

https://codereview.chromium.org/2907743003/diff/40001/net/quic/core/crypto/cr...
File net/quic/core/crypto/crypto_handshake_message.cc (right):

https://codereview.chromium.org/2907743003/diff/40001/net/quic/core/crypto/cr...
net/quic/core/crypto/crypto_handshake_message.cc:103: memcpy(&tag,
it->second.data() + i * sizeof(tag), sizeof(tag));
On 2017/06/12 20:32:31, Nico wrote:
> A few short weeks later, I suddenly remembered this CL and that I forgot to
> mention that you could've used
>
https://cs.chromium.org/chromium/src/base/bit_cast.h?type=cs&q=bit_cast+packa...
> here if you can depend on base.

Ooo! That's good to know. We can depend on base (though perhaps via a wrapper)
and it looks like we have this function internally too. There are quite a few
places where we do memcpys like this which would also be candidates for
bit_cast.

Thanks!