Closed Bug 1597137 Opened 10 months ago Closed 10 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged][trr])

Attachments

(2 files)

No description provided.

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.
Blocks: 1591724
  • 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

Blocks: 1565004
Pushed by valentin.gosu@gmail.com:
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
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

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:
Attachment #9110282 - Flags: approval-mozilla-beta?
Attachment #9109306 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Attachment #9110282 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9109306 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
QA Whiteboard: [qa-triaged]
Duplicate of this bug: 1598825

No dot release planned, wontfix for 71.

You need to log in before you can comment on or make changes to this bug.