1597137 - Don't load TRR entries from the cache if they are part of the excluded list

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect, P2)

Comment 1

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

Attachment: Bug 1597137 - Don't load TRR entries from the cache if they are part of the excluded list. r=dragana

This patch fixes two issues where we may mistakenly load a TRR record even though the host is part of the excluded-domains list:

1. If a.com is part of the excluded domains, but b.com is not, then when we first resolve b.com using TRR, the server may also push the record for a.com; Previously we didn't check if the pushed record is also excluded, which could lead us to load it from the TRR cache.
2. If b.com is resolved using TRR, but later b.com is added to the excluded-domains list, we may mistakenly load b.com from the TRR cache, even though we should use platform DNS for it.

Comment 2

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

Attachment: Bug 1597137 - Make sure that variables touched from IsExcludedFromTRR are locked r=dragana

• Since IsExcludedFromTRR may be called from multiple threads we should make sure that the member variables it uses are either atomic or locked.
• Makes IsTRRBlacklisted call IsExcludedFromTRR instead of querying variables again.
• Removes hardcoded checks for localhost and .local, since these are included in network.trr.builtin-excluded-domains

Depends on D53354

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/305ae4a012cc
Don't load TRR entries from the cache if they are part of the excluded list. r=dragana
https://hg.mozilla.org/integration/autoland/rev/70c76c5cf64e
Make sure that variables touched from IsExcludedFromTRR are locked r=dragana

Comment 4

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/305ae4a012cc
https://hg.mozilla.org/mozilla-central/rev/70c76c5cf64e

Comment 5

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

Comment on attachment 9110282 [details]
Bug 1597137 - Make sure that variables touched from IsExcludedFromTRR are locked r=dragana

Beta/Release Uplift Approval Request

• User impact if declined: Data-race when a network change event occurs and a DNS request is in progress on another thread. Possible crash.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: This bug is almost impossible to manually reproduce - but with millions of people running it it's likely it will cause crashes.
  QA is needed to mitigate the risk of causing regressions with this uplift. Checks should be performed that things behave normally with TRR turned on.
• List of other uplifts needed: Bug 1567346, Bug 1596234
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): There is a fair amount of risk given this is late in the release cycle.

Alternatives:
Backout bug 1582472

• String changes made/needed:

Comment 6

[Pascal Chevrel:pascalc]

Comment on attachment 9110282 [details]
Bug 1597137 - Make sure that variables touched from IsExcludedFromTRR are locked r=dragana

We don't have any beta left. This just landed in nightly, we are building RC this Monday and this looks too risky to me for the upcoming release (+ we don't have manual QA over the week end to test it anyway). As a P2, I think it should ride the 72 train (and potentially be uplifted in a 71 dot release). Marking as fix-optional in case we evaluate it for a dot release in December.

Comment 8

[Pascal Chevrel:pascalc]

No dot release planned, wontfix for 71.