fake_dns tool for Android.

fake_dns tool for Android (fixed DEPS).

It runs on device to resolves any host name to 127.0.0.1.
With it and the forwarder, all HTTP requests are redirected to our replay server
running on the host machine.

[Xianzhu, 2012-02-15 18:27:03]

Hi, Will,

Would you please review the fake_dns code which is mainly network code?

Thanks,
Xianzhu

[willchan no longer on Chromium, 2012-02-15 18:29:10]

No I won't, but I'll send it to someone who will :)

Syzmon: punt back to me if you need to, but I'm trying to shed load :) And
you're probably better versed in DNS stuff than I now.

[Xianzhu, 2012-02-15 18:33:57]

I'm changing sendto() to a loop. Will upload a new patch set soon.

[Xianzhu, 2012-02-15 18:45:13]

On 2012/02/15 18:33:57, Xianzhu wrote:
> I'm changing sendto() to a loop. Will upload a new patch set soon.

Done.

[szym, 2012-02-15 21:44:04]

This looks good and I appreciate the leanness. However, there's a lot that could
be reused from net/base/big_endian.h and net/dns/dns_protocol.h. 

I think it would be great if you used MessageLoopForIO to make it asynchronous
and portable, but I'll leave that choice up to you.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
File tools/android/fake_dns/fake_dns.cc (right):

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:22: #include "net/base/dns_util.h"
What is this needed for? If only kClassIN, then replace with
#include "net/dns/dns_protocol.h"

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:27: const size_t kMaxMessageSize = 512;
net::dns_protocol::kMaxUDPSize

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:28: const size_t kHeaderSize = 12;
sizeof(net::dns_protocol::Header)

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:32: const uint16 kDNS_A = 1;
net::dns_protocol::kTypeA

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:41: const size_t kAnswerSizeV6 = 12 +
sizeof(in6_addr);
net::kIPv4AddressSize  (in net/base/net_util.h)

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:44: // then the offset of the qname which
just follows the header.
It would help to specifically say that this is referring to the name in the
query. Perhaps call it kPointerToQueryName.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:47: const uint32 kTTL = 86400;
Comment that this equals 1 day.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:57: uint16 getU16(const char* buffer) {
See "net/base/big_endian.h" for this and the next two.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:73: bool SendPacketTo(int sock,
I think it wouldn't be much effort to make it asynchronous by reusing even more
from base/ and net/.

Take a look at "net/tools/testserver/run_testserver.cc" or
"net/curvecp/test_server.*". Once you create a MessageLoopForIO, you can use
"net/udp/udp_server_socket.h".

That said I understand if you want to keep it lean and simple, although if you
did use net/ then fake_dns would be usable on all our platforms as well.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:79: while (bytes_remaining > 0) {
This loop should not be necessary for datagram sockets. The receiver will have
no way to collect all the partial sends into the response. In case the kernel
decides to fragment the IP packet, there will only be one UDP header, and it
will not indicate a partial send to the caller.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:93: void SendEmptyResponse(int sock, const
sockaddr_in& client_addr, uint16 id) {
Rename to SendRefusedResponse. "Empty" suggests it's NOERROR but without
answers.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:97: putU16(response + 2, 0x8585);  // QR:1,
AA:1, RD:1, RA:1, RCODE:5 (Refused).
With dns_protocol this would be kFlagResponse | kFlagAA | kFlagRD | kFlagRA |
kRcodeREFUSED.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:113: putU16(response + 2, 0x8580);  // QR:1,
AA:1, RD:1, RA:1, RCODE:0 (No error).
As above.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:117: putU16(response + 10, 0);  // ARCOUNT
See BigEndianWriter in "net/base/big_endian.h".

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:128: putU16(answer + 10, 4);
As above.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:142: SendEmptyResponse(sock, client_addr, 0);
If id is set to 0, the client will ignore this, so I don't think it helps to
send back REFUSED, unless you plan to run tcpdump on the device and this will
help diagnostics. If that's the case, a comment would help.

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:147: uint16 flags = getU16(request + 2);
See BigEndianReader in "net/base/big_endian.h"

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:152: if ((flags & 0xf800) || qdcount != 1 ||
ancount || nscount || arcount) {
0xf800 looks arbitrary. Maybe:
const uint16 kAllowedFlags = 0x07ff;
if ((flags & ~kAllowedFlags) || ...

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:164: if (request[size - 5]) {
This is only a partial check. You're already including "net/base/dns_util.h" so
how about DNSDomainToString? Another option is DnsRecordParser::ReadName in
"net/dns/dns_response.h" (you can pass out = NULL).

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:196: perror("create socket");
Why perror here and LOG(ERROR) above?

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:206: setsockopt(sock, SOL_SOCKET,
SO_REUSEADDR, &reuse_addr, sizeof(reuse_addr));
I don't think SO_REUSEADDR is necessary. AFAIK, TIME_WAIT applies to STREAM
sockets only, but if you've already tried removing it and got "address already
in use", then obviously I'm wrong.

[szym, 2012-02-15 21:46:43]

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
File tools/android/fake_dns/fake_dns.cc (right):

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:147: uint16 flags = getU16(request + 2);
On 2012/02/15 21:44:04, szym wrote:
> See BigEndianReader in "net/base/big_endian.h"

There's also the option to use dns_protocol::Header with ntohs as in
"net/dns/dns_response.cc"

[szym, 2012-02-15 21:57:23]

Ok, after all this nitpicking, here's an actual bug. See below. Sorry about all
the noise, I'm obviously quite excited to do my first serious review ;-)

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
File tools/android/fake_dns/fake_dns.cc (right):

http://codereview.chromium.org/9401014/diff/6005/tools/android/fake_dns/fake_...
tools/android/fake_dns/fake_dns.cc:128: putU16(answer + 10, 4);
BUG: This should be |response_size|.

[Xianzhu, 2012-02-16 01:19:36]

Thanks for review!

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
File tools/android/fake_dns/fake_dns.cc (right):

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:22: #include "net/base/dns_util.h"
On 2012/02/15 21:44:04, szym wrote:
> What is this needed for? If only kClassIN, then replace with
> #include "net/dns/dns_protocol.h"

Now use net/dns/dns_protocol.h, net/base/net_util.h and net/base/big_endian.h

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:27: const size_t kMaxMessageSize = 512;
On 2012/02/15 21:44:04, szym wrote:
> net::dns_protocol::kMaxUDPSize

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:28: const size_t kHeaderSize = 12;
On 2012/02/15 21:44:04, szym wrote:
> sizeof(net::dns_protocol::Header)

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:32: const uint16 kDNS_A = 1;
On 2012/02/15 21:44:04, szym wrote:
> net::dns_protocol::kTypeA

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:41: const size_t kAnswerSizeV6 = 12 +
sizeof(in6_addr);
On 2012/02/15 21:44:04, szym wrote:
> net::kIPv4AddressSize  (in net/base/net_util.h)

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:44: // then the offset of the qname which
just follows the header.
On 2012/02/15 21:44:04, szym wrote:
> It would help to specifically say that this is referring to the name in the
> query. Perhaps call it kPointerToQueryName.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:47: const uint32 kTTL = 86400;
On 2012/02/15 21:44:04, szym wrote:
> Comment that this equals 1 day.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:57: uint16 getU16(const char* buffer) {
On 2012/02/15 21:44:04, szym wrote:
> See "net/base/big_endian.h" for this and the next two.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:73: bool SendPacketTo(int sock,
On 2012/02/15 21:44:04, szym wrote:
> I think it wouldn't be much effort to make it asynchronous by reusing even
more
> from base/ and net/.
> 
> Take a look at "net/tools/testserver/run_testserver.cc" or
> "net/curvecp/test_server.*". Once you create a MessageLoopForIO, you can use
> "net/udp/udp_server_socket.h".
> 
> That said I understand if you want to keep it lean and simple, although if you
> did use net/ then fake_dns would be usable on all our platforms as well.
> 

We don't want to introduce much dependency to Chromium so that this tool can be
easily build without Chromium in the future. It seems OK to depend on some data
type and constants and simple utilities because it's easy to duplicate.
Introducing MessageLoopForIO dependency seems too heavy.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:79: while (bytes_remaining > 0) {
On 2012/02/15 21:44:04, szym wrote:
> This loop should not be necessary for datagram sockets. The receiver will have
> no way to collect all the partial sends into the response. In case the kernel
> decides to fragment the IP packet, there will only be one UDP header, and it
> will not indicate a partial send to the caller.

Changed back to non-loop version.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:93: void SendEmptyResponse(int sock, const
sockaddr_in& client_addr, uint16 id) {
On 2012/02/15 21:44:04, szym wrote:
> Rename to SendRefusedResponse. "Empty" suggests it's NOERROR but without
> answers.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:97: putU16(response + 2, 0x8585);  // QR:1,
AA:1, RD:1, RA:1, RCODE:5 (Refused).
On 2012/02/15 21:44:04, szym wrote:
> With dns_protocol this would be kFlagResponse | kFlagAA | kFlagRD | kFlagRA |
> kRcodeREFUSED.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:113: putU16(response + 2, 0x8580);  // QR:1,
AA:1, RD:1, RA:1, RCODE:0 (No error).
On 2012/02/15 21:44:04, szym wrote:
> As above.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:117: putU16(response + 10, 0);  // ARCOUNT
On 2012/02/15 21:44:04, szym wrote:
> See BigEndianWriter in "net/base/big_endian.h".

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:128: putU16(answer + 10, 4);
On 2012/02/15 21:44:04, szym wrote:
> As above.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:128: putU16(answer + 10, 4);
On 2012/02/15 21:57:23, szym wrote:
> BUG: This should be |response_size|.

Good Catch! I think it should be the sizeof RDATA.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:142: SendEmptyResponse(sock, client_addr, 0);
On 2012/02/15 21:44:04, szym wrote:
> If id is set to 0, the client will ignore this, so I don't think it helps to
> send back REFUSED, unless you plan to run tcpdump on the device and this will
> help diagnostics. If that's the case, a comment would help.

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:147: uint16 flags = getU16(request + 2);
On 2012/02/15 21:44:04, szym wrote:
> See BigEndianReader in "net/base/big_endian.h"

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:147: uint16 flags = getU16(request + 2);
On 2012/02/15 21:46:43, szym wrote:
> On 2012/02/15 21:44:04, szym wrote:
> > See BigEndianReader in "net/base/big_endian.h"
> 
> There's also the option to use dns_protocol::Header with ntohs as in
> "net/dns/dns_response.cc"

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:152: if ((flags & 0xf800) || qdcount != 1 ||
ancount || nscount || arcount) {
On 2012/02/15 21:44:04, szym wrote:
> 0xf800 looks arbitrary. Maybe:
> const uint16 kAllowedFlags = 0x07ff;
> if ((flags & ~kAllowedFlags) || ...

Done.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:164: if (request[size - 5]) {
On 2012/02/15 21:44:04, szym wrote:
> This is only a partial check. You're already including "net/base/dns_util.h"
so
> how about DNSDomainToString? Another option is DnsRecordParser::ReadName in
> "net/dns/dns_response.h" (you can pass out = NULL).

We don't actually parse the QNAME, so it seems unnecessary to fully check the
validity. Also ddding full check would introduce more dependency and run-time
cost.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:196: perror("create socket");
On 2012/02/15 21:44:04, szym wrote:
> Why perror here and LOG(ERROR) above?
Because this is in the main process which can be seen by the user. The
LOG(ERROR)s are in child process which doesn't have standard output (closed in
SpawnDaemon to avoid adb stall). LOG(ERROR) will output to Android adb log.

https://chromiumcodereview.appspot.com/9401014/diff/6005/tools/android/fake_d...
tools/android/fake_dns/fake_dns.cc:206: setsockopt(sock, SOL_SOCKET,
SO_REUSEADDR, &reuse_addr, sizeof(reuse_addr));
On 2012/02/15 21:44:04, szym wrote:
> I don't think SO_REUSEADDR is necessary. AFAIK, TIME_WAIT applies to STREAM
> sockets only, but if you've already tried removing it and got "address already
> in use", then obviously I'm wrong.

Verified you are right.

[szym, 2012-02-16 02:30:17]

Cool! LGTM

[commit-bot: I haz the power, 2012-02-16 17:11:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/9401014/8

[commit-bot: I haz the power, 2012-02-16 18:32:51]

Change committed as 122320