Closed Bug 1439067 Opened 3 years ago Closed 3 years ago

trr null deref

Categories

(Core :: Networking, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mcmanus, Assigned: bagder)

References

Details

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

Attachments

(1 file)

https://crash-stats.mozilla.com/report/index/9facb04f-d306-4ee3-8ccc-a53b00180217
https://crash-stats.mozilla.com/report/index/5e7a3194-f394-48ed-9b91-a76980180217

crash loop on startup. mode was trr-only

obv can be null checked, but should also explain how the service is null
Assignee: nobody → daniel
Blocks: 1434852
Since we're here, TRR::mTRRService is just a raw pointer a TRRService. Maybe we could always use gTRRService and null check it.
(In reply to Valentin Gosu [:valentin] from comment #1)
> Since we're here, TRR::mTRRService is just a raw pointer a TRRService. Maybe
> we could always use gTRRService and null check it.

yeah.

I still don't understand how this happens
got it - the dns service can go up and down when it reconfigures.. when it does so the trr service also goes up and down.

as trrservice is only used on the main thread this should be safe enough to null check. But its done asynchronously which answers the question of how it got to be null after dns had started.

fwiw this only reprod on windows for me.
Priority: -- → P1
Whiteboard: [necko-triaged][trr]
Duplicate of this bug: 1439098
Making use of the global pointer won't help as it can basically change at any point in time, right?

Isn't it a better solution to simply use mTRRService refcounted in the TRR object so that it can hold on to the existing instance for as long it runs the TRR request?

diff --git a/netwerk/dns/TRR.h b/netwerk/dns/TRR.h
index 55ce9701bcd5..9fc0cdb8ea65 100644
--- a/netwerk/dns/TRR.h
+++ b/netwerk/dns/TRR.h
@@ -135,11 +135,11 @@ public:
   void Cancel();
   enum TrrType Type() { return mType; }
   nsCString mHost;
   RefPtr<nsHostRecord> mRec;
   RefPtr<AHostResolver> mHostResolver;
-  TRRService *mTRRService;
+  RefPtr<TRRService> mTRRService;
 
 private:
   ~TRR() = default;
   nsresult SendHTTPRequest();
   nsresult DohEncode(nsCString &target);
Flags: needinfo?(valentin.gosu)
Yes, that's probably better.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8952113 [details]
bug 1439067 - let TRR access TRRService through the null-checked global.

https://reviewboard.mozilla.org/r/221350/#review227184

Before we land this we should run the DNS/TRR unit tests to make sure we don't leak anything.
Attachment #8952113 - Flags: review?(valentin.gosu) → review+
Agreed. Try run, which looks similar to the run before this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=452748a4620250fee36ff6fd7529d7b48499103d
(In reply to Daniel Stenberg [:bagder] from comment #5)
> Making use of the global pointer won't help as it can basically change at
> any point in time, right?
>

I believe it is only accessed on the main thread - so null checking a global should be sufficient. can it be used off the main thread?