Closed Bug 1138242 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Networking: DNS, defect)

39 Branch
All
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- affected
firefox41 --- fixed

People

(Reporter: sworkman, Assigned: valentin)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Depends on: 1093983
Work in progress. Applies, but does not build yet.
Assignee: nobody → sworkman
Status: NEW → ASSIGNED
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.
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)
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!
Flags: needinfo?(lhenry)
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)
Assignee: sworkman → nobody
Status: ASSIGNED → NEW
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!
Attached patch RequestAandAAAA.diff (obsolete) — Splinter Review
Attachment #8600523 - Attachment is obsolete: true
Attachment #8603685 - Flags: review?(honzab.moz)
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+
Attachment #8603685 - Attachment is obsolete: true
Attachment #8607543 - Attachment is obsolete: true
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)
Blocks: 1167552
https://hg.mozilla.org/mozilla-central/rev/7f2f23148b69
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.