1138242 - DNS: Request A and AAAA record types rather than ANY when obtaining TTL on Windows

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

Bug 1093983 highlighted concerns with the use of the 'ANY' record type in DNS. We previously used this to obtain cached records from DNS resolvers in order to get the TTL for each record. Because of the concerns mentioned in that bug, we are adjusting the code to request A and AAAA records instead.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.0 Use sequential A and AAAA requests to get DNS record TTLs on Windows

Work in progress. Applies, but does not build yet.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.1 Use sequential A and AAAA requests to get DNS record TTLs on Windows

Now builds. But m-c is crashing for me on Windows, even without the patch. Will come back to this tomorrow.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:  Tracking this for 39 since we are also tracking bug  	1093983.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Steve, does it need to be fixed in the same releases that bug 1093983 ships in or can it wait to be fixed in 39? Thanks!

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Liz - should be fine to wait. We'll try to speed it through the channels, but it isn't needed for 36.

Comment 6

[Patrick McManus [:mcmanus]]

Liz, I don't think this bug needs to be tracked.. the negative impact was resolved in bug 1093983.. this bug is about shipping a revised strategy.. while its desirable to do that asap (potentially via backport) it isn't linked to any particular release.

maybe untrack?

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks for the clarification!

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57991807ac0d

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Use sequential A and AAAA requests to get DNS record TTLs on Windows

Uploading most recent patch I have. Slight chance it will need rebasing, but it's mostly in GetAddrInfo.cpp which is quite isolated.

Valentin: Pat suggested I ask you to take this forward as it's been languishing in my patch queue and I'm not going to have time for it. Let me know if you can do it and if you need any help.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Sure, Steve, I can take this. Besides rebasing, could you tell me if there's anything else left to implement?

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Valentin - you're awesome!

Rebasing and some debugging. Two ways to debug:

1. You should see more A and AAAA queries on Wireshark.
2. If you enabled NSPR_LOGGING=GetAddrInfo:5 you should see ttls that vary. See https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/GetAddrInfo.cpp?from=GetAddrInfo.cpp&case=true#407

Reminder: this is only on Windows. You shouldn't see any non-default ttls on other platforms (because the code shouldn't be built!)

Let me know if you need any more help. Thanks again!

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: RequestAandAAAA.diff

Comment 13

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=591569057f4e

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8603685 [details] [diff] [review]
RequestAandAAAA.diff

Honza is sick this week.

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8603685 [details] [diff] [review]
RequestAandAAAA.diff

Review of attachment 8603685 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor things. Otherwise r=me.

::: netwerk/dns/GetAddrInfo.cpp
@@ +153,5 @@
>    return NS_OK;
>  }
>  
> +// If successful, returns in aResult a TTL value that is smaller or
> +// equal with the one already there. Gets the TTL value by calling 

nit: end of line whitespace

@@ +160,4 @@
>  static MOZ_ALWAYS_INLINE nsresult
> +_GetMinTTLForRequestType_Windows(DnsapiInfo * dnsapi, const char* aHost,
> +                                 uint16_t aRequestType, unsigned int* aResult)
> +{

Just in case, please add some asserts/nullptr handling for these params.

@@ +177,5 @@
> +        aHost, status, aRequestType);
> +    return NS_ERROR_FAILURE;
> +  } else if (status != NOERROR) {
> +    LOG_WARNING("DnsQuery_A failed with status %X.\n", status);
> +    return NS_ERROR_FAILURE;;

nit: NS_ERROR_UNEXPECTED, just because this would be a weird case. _FAILURE is also fine though.

@@ +181,5 @@
> +    return NS_ERROR_FAILURE;;
> +  }
> +
> +  PDNS_RECORDA curRecord = dnsData;
> +  for (; curRecord; curRecord = curRecord->pNext) {

Can curRecord be declared in the for statement?

for (PDNS_RECORDA curRecord = dnsData ....) {

Looks like it's only used in the for loop.

@@ +200,5 @@
> +  return NS_OK;
> +}
> +
> +static MOZ_ALWAYS_INLINE nsresult
> +_GetTTLData_Windows(const char* aHost, uint16_t* aResult, uint16_t aAddressFamily)

You should be able to return if aAddressFamily is invalid here too.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Attachment: Use sequential A and AAAA requests to get DNS record TTLs on Windows

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Attachment: Use sequential A and AAAA requests to get DNS record TTLs on Windows

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Hey Patrick,
Steve said I should point out to you that he reviewed this, since he also wrote the original version of the patch.

Comment 19

[Patrick McManus [:mcmanus]]

(In reply to Valentin Gosu [:valentin] from comment #18)
> Hey Patrick,
> Steve said I should point out to you that he reviewed this, since he also
> wrote the original version of the patch.

As long as you've both read the whole patch we've achieved the objective of code review :)

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2f23148b69

Comment 21

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/7f2f23148b69