Closed Bug 1190502 Opened 4 years ago Closed 4 years ago

RESOLVE_DISABLE_IPV4 returns A records

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- affected
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: drno, Assigned: valentin)

References

Details

Attachments

(1 file, 1 obsolete file)

In the WebRTC code we request IPv6 only DNS lookups 
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nriceresolver.cpp#172
so we are expecting to get only results for AAAA entries. But it turns out we actually get back A record entries.
The good case when the DNS name has AAAA record only looks in the logs like this:

(ice/DEBUG) ICE(P1): resolved candidate srflx(IP6:[2620:101:80fc:224:bae8:56ff:fe2d:3d96]:55737/UDP|naboo6.rfc3261.net:3478). addr=IP6:[2a01:238:429e:6b00:6188:46:9836:e1b4]:3478/UDP
...
(ice/WARNING) ICE(P1): failed to resolve candidate srflx(IP4:10.252.26.155:52644/UDP|naboo6.rfc3261.net:3478)

In this case the DNS resolve with RESOLVE_DISABLE_IPV6 failed and returned an empty answer as expected.


But if I point the DNS name to an AAAA record we get this (from our unit test logs):

(ice/DEBUG) ICE(P1): resolved candidate srflx(IP4:10.252.26.155:54259/UDP|naboo4.rfc3261.net:3478). addr=IP4:85.214.41.172:3478/UDP
...
(ice/DEBUG) ICE(P1): resolved candidate srflx(IP6:[2620:101:80fc:224:bae8:56ff:fe2d:3d96]:57726/UDP|naboo4.rfc3261.net:3478). addr=IP4:85.214.41.172:3478/UDP

Note: our code happens to do a DNS resolve requests with RESOLVE_DISABLE_IPV6 and RESOLVE_DISABLE_IPV4 in parallel or random order. I have not verified if that triggers this behavior.

The comment in the IDL suggests that this behavior is not expected:
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/netwerk/dns/nsIDNSService.idl#l163

Note 2: I'm filling this in HTTP only, because bug 725587 which introduced RESOLVE_DISABLE_IPV4 was filed in that category.
See Also: → 1190559
Flags: needinfo?(honzab.moz)
Tested all combinations:

DNS\option| DISABLE_IPV6 | DISABLE_IPV4
----------+--------------+-------------
AAAA only |    none      |   AAAA
----------+--------------+-------------
A & AAAA  |     A        |   AAAA
----------+--------------+-------------
A only    |     A        |    A (this should be 'none')

So only the scenario where the DNS entry has only an A record DISABLE_IPV4 returns the wrong result. All other combinations work as expected.

I also ran our tests with an additional HasMore() check (because we always only look at the first entry) to rule out that we get A and AAAA records. But everything looks sane on that front. Multiple A records show up as expected, and at no point A and AAAA get returned together.
I think the problem may be here:
https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsHostResolver.cpp#1411

It says it should remove IPv4 records manually, but from what I can tell it doesn't.
nsHostResolver::ThreadFunc should not override addressFamily with PR_AF_UNSPEC
for IPv6 since GetAddrInfo.cpp::GetAddrInfo() can handle PR_AF_INET6.
_GetAddrInfo_Portable does this before calling PR_GetAddrInfoByName and
creates the AddrInfo with a disableIPv4 flag if necessary.
Attachment #8644051 - Flags: review?(mcmanus)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Comment on attachment 8644051 [details] [diff] [review]
RESOLVE_DISABLE_IPV4 returns A records

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

I think this is right.. the ipv4 filter happens in getaddrinfo now and so the code you removed is a legacy of before the win/ttl stuff. Still, I'd like steve to review if he has a chance.

Steve - its short!
Attachment #8644051 - Flags: review?(sworkman)
Attachment #8644051 - Flags: review?(mcmanus)
Attachment #8644051 - Flags: feedback+
Comment on attachment 8644051 [details] [diff] [review]
RESOLVE_DISABLE_IPV4 returns A records

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

I just verified that this patch results in the expected behavior in our unit tests now for all the combinations of DISABLE_IPV4 and DISABLE_IPV6 (note: we don't make resolver queries without any of these two flags, so I haven't verified that).

Awesome! Thanks a lot for the quick turn-around time.
Attachment #8644051 - Flags: review+
Comment on attachment 8644051 [details] [diff] [review]
RESOLVE_DISABLE_IPV4 returns A records

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

Sorry, obviously meant to provide only feedback not review.
Attachment #8644051 - Flags: review+ → feedback+
Comment on attachment 8644051 [details] [diff] [review]
RESOLVE_DISABLE_IPV4 returns A records

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

r=me with the 2nd test script for IPv6 added.

Thanks folks!

::: netwerk/test/unit/test_dns_disable_ipv4.js
@@ +30,5 @@
> +
> +function run_test() {
> +  do_test_pending();
> +  try {
> +    dns.asyncResolve("localhost", Ci.nsIDNSService.RESOLVE_DISABLE_IPV4, listener, null);

It would be nice for completeness if you also tested disabling IPv6 ;)

I'd be fine with a second js file like this one, just adjusted for IPv6.
Attachment #8644051 - Flags: review?(sworkman) → review+
Backed out together with bug 1187159: https://hg.mozilla.org/integration/mozilla-inbound/rev/56124e9589e1

23:29:41     INFO -  ==2020==ERROR: LeakSanitizer: detected memory leaks
23:29:41     INFO -  Direct leak of 96 byte(s) in 3 object(s) allocated from:
23:29:41     INFO -      #0 0x472231 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
23:29:41     INFO -      #1 0x48b79d in moz_xmalloc /builds/slave/m-in-l64-asan-0000000000000000/build/src/memory/mozalloc/mozalloc.cpp:83
23:29:41     INFO -      #2 0x7fc61d0e1dfd in operator new /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/netwerk/base/../../dist/include/mozilla/mozalloc.h:186
23:29:41     INFO -      #3 0x7fc61d0e1dfd in mozilla::net::GetLoadContextInfo(nsIChannel*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/netwerk/base/LoadContextInfo.cpp:68
23:29:41     INFO -      #4 0x7fc61d5833a6 in mozilla::net::nsHttpChannel::BeginConnect() /builds/slave/m-in-l64-asan-0000000000000000/build/src/netwerk/protocol/http/nsHttpChannel.cpp:5215
23:29:41     INFO -      #5 0x7fc61d58599f in mozilla::net::nsHttpChannel::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, nsresult) /builds/slave/m-in-l64-asan-0000000000000000/build/src/netwerk/protocol/http/nsHttpChannel.cpp:5441
23:29:41     INFO -  -----------------------------------------------------
23:29:41     INFO -  Suppressions used:
23:29:41     INFO -    count      bytes template
23:29:41     INFO -       40        986 libc.so
23:29:41     INFO -      832      26576 nsComponentManagerImpl
23:29:41     INFO -       69       9384 mozJSComponentLoader::LoadModule
23:29:41     INFO -        1        384 pixman_implementation_lookup_composite
23:29:41     INFO -     7862     372704 libfontconfig.so
23:29:41     INFO -        2         48 PORT_Strdup_Util
23:29:41     INFO -        2         88 _PR_Getfd
23:29:41     INFO -      134      56872 libcairo.so
23:29:41     INFO -        1         32 libdl.so
23:29:41     INFO -     2313      41164 libglib-2.0.so
23:29:41     INFO -        2        144 libpulse.so
23:29:41     INFO -        1         40 libpulsecommon-1.1.so
23:29:41     INFO -        2         56 libresolv.so
23:29:41     INFO -  -----------------------------------------------------
23:29:41     INFO -  SUMMARY: AddressSanitizer: 96 byte(s) leaked in 3 allocation(s).
23:29:41     INFO -  TEST-INFO | Main app process: exit 0
23:29:41  WARNING -  TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at mozilla::net::GetLoadContextInfo, mozilla::net::nsHttpChannel::BeginConnect, mozilla::net::nsHttpChannel::OnProxyAvailable
https://hg.mozilla.org/mozilla-central/rev/abf9d5490266
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Flags: needinfo?(honzab.moz)
Landed patch - r+ in comment 7

Approval Request Comment
[Feature/regressing bug #]:
unknown

[User impact if declined]:
Some features may not work properly.
A call to asyncResolve will not return the expected type of IP addresses.

[Describe test coverage new/current, TreeHerder]:
Added a unit test to make sure this feature does not regress.

[Risks and why]: 
Low risk. This change does not make big code changes.

[String/UUID change made/needed]:
None.
Attachment #8644051 - Attachment is obsolete: true
Attachment #8647499 - Flags: review+
Attachment #8647499 - Flags: approval-mozilla-beta?
Attachment #8647499 - Flags: approval-mozilla-aurora?
Valentin, is this a regression? If yes, any idea when it started.

> Low risk. This change does not make big code changes.
Even it does not change a lot of code, DNS resolution is a pretty important feature in a browser. Why do you think it is low risk? Thanks
Flags: needinfo?(valentin.gosu)
It seems this regression was introduced along with 4d21e0728967 (bug 1067679)
The call to PR_GetAddrInfoByName in ThreadFunc was changed to GetAddrInfo - which now performs the filtering that ThreadFunc used to. However, the lines in ThreadFunc which changed the flags weren't removed, which meant that a request for PR_AF_INET6 addresses was replaced with one for PR_AF_UNSPEC and caused this bug.

This patch only makes sure that the flags are passed unaltered, and makes no changes which might otherwise change the code flow. That is why I consider it low risk.
Flags: needinfo?(valentin.gosu)
Any idea why we didn't see that sooner?
Why this cannot ride the train from aurora?
Blocks: 1067679
(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Any idea why we didn't see that sooner?

The call still returned valid IP addresses, but if you requested IPv6 only, it would return both IPv4 and IPv6. Unless the consumer specifically checks for the requested type of IP addresses, this misbehaviour is very difficult to detect.

> Why this cannot ride the train from aurora?

Patrick suggested I nominate it to be uplifted, but I think we could not uplift to beta if it seems too risky.
Flags: needinfo?(mcmanus)
(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Any idea why we didn't see that sooner?

If you search for RESOLVE_DISABLE_IPV4 in our code base it looks to me like nobody is actually using that flag, until we landed IPv6 support in WebRTC.

> Why this cannot ride the train from aurora?

WebRTC landed IPv6 support in 42 which now depends on this fix. So we either need this fix in Aurora or we will have land our work around in bug 1190559 straight into Aurora (which is probably less risky at that would only affect WebRTC). But we do not need it in Beta as we don't use that feature there yet.
(In reply to Valentin Gosu [:valentin] from comment #18)

> 
> Patrick suggested I nominate it to be uplifted, but I think we could not
> uplift to beta if it seems too risky.

This is impacts our happy eyeballs support. So users on networks that prefer V6 (a strong trend, especially in asia) are going to get subpar results without it.
Flags: needinfo?(mcmanus)
Comment on attachment 8647499 [details] [diff] [review]
RESOLVE_DISABLE_IPV4 returns A records

Approving for uplift to Aurora. For Beta-, please see comment 18 and 19.
Attachment #8647499 - Flags: approval-mozilla-beta?
Attachment #8647499 - Flags: approval-mozilla-beta-
Attachment #8647499 - Flags: approval-mozilla-aurora?
Attachment #8647499 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.