Consistently suggest ephemeral port for QUIC client

net/quic/port_suggester.h

Index: net/quic/port_suggester.h
diff --git a/net/quic/port_suggester.h b/net/quic/port_suggester.h
new file mode 100644
index 0000000000000000000000000000000000000000..52655648c0f1cc204d9ea40a4c82f003f8234580
--- /dev/null
+++ b/net/quic/port_suggester.h
@@ -0,0 +1,51 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_QUIC_PORT_SUGGESTER_H_
+#define NET_QUIC_PORT_SUGGESTER_H_
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/sha1.h"
+#include "net/base/net_export.h"
+
+namespace net {
+
+class HostPortPair;
+
+// We provide a pseudo-random number generator that is always seeded the same
+// way for a given destination host-port pair.  The generator is used to
+// consistently suggest (for that host-port pair) an ephemeral source port,
+// and hence increase the likelihood that a server's load balancer will direct
+// a repeated connection to the same server (with QUIC, further increasing the
+// chance of connection establishment with 0-RTT).
+class NET_EXPORT PortSuggester
+    : public base::RefCountedThreadSafe<PortSuggester> {

[Ryan Hamilton, 2013/12/07 01:46:14]

Does this really need to be thread safe?  Almost nothing in net/ is thread-safe,
so that makes me think this does not need to be.

[jar (doing other things), 2013/12/07 03:12:18]

Done.

+ public:
+  // |per_profile_randomness| is used to prevent (reduce probability of) one
+  // profile's suggestions from matching that of another.
+  PortSuggester(const HostPortPair& server, uint64 per_profile_randomness);

[Ryan Hamilton, 2013/12/07 01:46:14]

nit: can per_profile_randomness simply be renamed "seed".  This class knows
nothing about profiles, it's really just using this to help seed the generator,
right?

[jar (doing other things), 2013/12/07 03:12:18]

Done.

[Ryan Hamilton, 2013/12/07 23:21:54]

Looks like it's still called per_profile_randomness.  Please rename it to seed.

[jar (doing other things), 2013/12/08 01:38:36]

My mistake.  I thought I did it.  DONE now.

+
+  // Generate a pseudo-random int in the inclusive range from |min| to |max|.
+  // Will (probably) return different numbers when called repeatedly.
+  int SuggestPort(int min, int max);
+
+ private:
+  friend class base::RefCountedThreadSafe<PortSuggester>;
+
+  virtual ~PortSuggester();
+
+   union {
+     unsigned char hash_bytes[base::kSHA1Length];
+     uint64 first_uint64;
+     int first_int;

[wtc, 2013/12/07 01:03:38]

The indentation of these four lines seem off by one.

[jar (doing other things), 2013/12/07 03:12:18]

Done.

+  } seed_;

[Ryan Hamilton, 2013/12/07 01:46:14]

This union seems really subtle and not terribly readable.  Did you consider
simply having a unit64 seed_?  When you evolve the seed you could then do:

  unsigned char hash[base::kSHA1Length];
  base::SHA1HashBytes(reinterpret_cast<unsigned char*>(&seed_), sizeof(seed_),
                      hash);
  memcpy(&seed_, hash, sizeof(seed_));

(This would make your seed_ member variable 8 bytes, instead of 20.  A whopping
12 byte savings :>)

[jar (doing other things), 2013/12/07 03:12:18]

I originally used reinterpret casts, but wtc noted that care was needed to avoid
alignment issues on some platform... so the union ended up being cleaner.

[Ryan Hamilton, 2013/12/07 04:52:22]

Hm.  I don't think there are alignment issues with a char*.  Think about it this
way.  I could have:

  char bytes[1024];

I should be totally able to say:

  int i = rand_int_between(0,1024);
  SHA1HashBytes(&bytes[i], 1, output);

Given this, it's clear that alignment can't really be a concern.  In any case,
SHA1HashBytes calls down to this code:

void SecureHashAlgorithm::Update(const void* data, size_t nbytes) {
  const uint8* d = reinterpret_cast<const uint8*>(data);

Notice that it takes a void* and immediately casts it to a char* (well, uint8,
but whatever :>).  If alignment were a concern, then that would be totally
unsafe.

Now, if you were going the other way, there could be problems:

reinterpret_cast<uint64*>(&bytes[i])

That's not guaranteed to align, so we need to memcpy in this case, as per the
last line in my snippet.

[jar (doing other things), 2013/12/07 21:09:28]

The alignment issue (see wtc comments in patch 6) was that 
char bytes[8];
inside the class is not guaranteed to be any more aligned than a char.  As a
result, when I try to XOR onto its bytes, or try to extract the first int by
casting using:

reinterpret_cast<int*>(bytes)

...there would be alignment trouble (potentially).  

The use of unions avoids all this.   I used to think union was just cuter... but
I'm now convinced (by wtc) that it also avoids the mis-aligned problems, and
I've seen it in tons of code.

[Ryan Hamilton, 2013/12/07 23:21:54]

Ah, yes, using reinterpret_cast to go from bytes to some wider data type has
definite issues. Thankfully, the other direction is not a problem so the code I
recommended (and that you have in your CL now) is safe.  Yes, the union does
align things to ensure safety, but since it took the analysis of an expert to
become convinced of that fact, I think it's more readable to avoid it.

+  int count_;  // Number of suggestions made.
+
+  DISALLOW_COPY_AND_ASSIGN(PortSuggester);
+};
+
+}  // namespace net
+
+#endif  // NET_QUIC_PORT_SUGGESTER_H_