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

RESOLVED FIXED in Firefox 41

Status

()

Core
Networking: DNS
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sworkman, Assigned: valentin)

Tracking

39 Branch
mozilla41
All
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 affected, firefox37 affected, firefox38 affected, firefox39 affected, firefox41 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
(Reporter)

Updated

3 years ago
Depends on: 1093983
(Reporter)

Comment 1

3 years ago
Created attachment 8571099 [details] [diff] [review]
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.
Assignee: nobody → sworkman
Status: NEW → ASSIGNED
(Reporter)

Comment 2

3 years ago
Created attachment 8571101 [details] [diff] [review]
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.
[Tracking Requested - why for this release]:  Tracking this for 39 since we are also tracking bug  	1093983.
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox39: --- → affected
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?
tracking-firefox38: --- → ?
tracking-firefox39: --- → +
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!
Flags: needinfo?(sworkman)
(Reporter)

Comment 5

3 years ago
Liz - should be fine to wait. We'll try to speed it through the channels, but it isn't needed for 36.
Flags: needinfo?(sworkman)
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?
Flags: needinfo?(lhenry)
OK, thanks for the clarification!
tracking-firefox36: ? → ---
tracking-firefox37: ? → ---
tracking-firefox38: ? → ---
tracking-firefox39: + → ---
Flags: needinfo?(lhenry)
(Reporter)

Comment 8

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57991807ac0d
(Reporter)

Comment 9

3 years ago
Created attachment 8600523 [details] [diff] [review]
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.
Attachment #8571099 - Attachment is obsolete: true
Attachment #8571101 - Attachment is obsolete: true
Flags: needinfo?(valentin.gosu)
(Reporter)

Updated

3 years ago
Assignee: sworkman → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 10

3 years ago
Sure, Steve, I can take this. Besides rebasing, could you tell me if there's anything else left to implement?
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
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!
(Assignee)

Comment 12

3 years ago
Created attachment 8603685 [details] [diff] [review]
RequestAandAAAA.diff
Attachment #8600523 - Attachment is obsolete: true
Attachment #8603685 - Flags: review?(honzab.moz)
(Assignee)

Comment 13

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=591569057f4e
(Assignee)

Comment 14

3 years ago
Comment on attachment 8603685 [details] [diff] [review]
RequestAandAAAA.diff

Honza is sick this week.
Attachment #8603685 - Flags: review?(honzab.moz) → review?(sworkman)
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.
Attachment #8603685 - Flags: review?(sworkman) → review+
(Assignee)

Comment 16

3 years ago
Created attachment 8607543 [details] [diff] [review]
Use sequential A and AAAA requests to get DNS record TTLs on Windows
(Assignee)

Updated

3 years ago
Attachment #8603685 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8607546 [details] [diff] [review]
Use sequential A and AAAA requests to get DNS record TTLs on Windows
(Assignee)

Updated

3 years ago
Attachment #8607543 - Attachment is obsolete: true
(Assignee)

Comment 18

3 years ago
Hey Patrick,
Steve said I should point out to you that he reviewed this, since he also wrote the original version of the patch.
Flags: needinfo?(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 :)
Flags: needinfo?(mcmanus)

Comment 20

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2f23148b69
(Assignee)

Updated

3 years ago
Blocks: 1167552

Comment 21

3 years ago
https://hg.mozilla.org/mozilla-central/rev/7f2f23148b69
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.