1190502 - RESOLVE_DISABLE_IPV4 returns A records

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Nils Ohlmeier [:drno]]

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.

Comment 1

[Nils Ohlmeier [:drno]]

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.

Comment 2

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

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.

Comment 3

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

Attachment: RESOLVE_DISABLE_IPV4 returns A records

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.

Comment 4

[Patrick McManus [:mcmanus]]

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!

Comment 5

[Nils Ohlmeier [:drno]]

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.

Comment 6

[Nils Ohlmeier [:drno]]

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.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/618c213ed8db

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56124e9589e1

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 11

[(no longer active)]

Actually backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc6b3d81906

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abf9d5490266

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/abf9d5490266

Comment 14

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

Attachment: RESOLVE_DISABLE_IPV4 returns A records

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.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

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

Comment 16

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

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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Any idea why we didn't see that sooner?
Why this cannot ride the train from aurora?

Comment 18

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

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

Comment 19

[Nils Ohlmeier [:drno]]

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

Comment 20

[Patrick McManus [:mcmanus]]

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

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4cbcd9fff58