1439067 - trr null deref

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Patrick McManus [:mcmanus]]

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

Comment 1

[Valentin Gosu [:valentin] (he/him) [Back on 21st of January]]

Since we're here, TRR::mTRRService is just a raw pointer a TRRService. Maybe we could always use gTRRService and null check it.

Comment 2

[Patrick McManus [:mcmanus]]

(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

Comment 3

[Patrick McManus [:mcmanus]]

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.

Comment 5

[Daniel Stenberg [:bagder]]

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);

Comment 6

[Valentin Gosu [:valentin] (he/him) [Back on 21st of January]]

Yes, that's probably better.

Comment 7

[Daniel Stenberg [:bagder]]

Attachment: bug 1439067 - let TRR access TRRService through the null-checked global.

Review commit: https://reviewboard.mozilla.org/r/221350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221350/

Comment 8

[Valentin Gosu [:valentin] (he/him) [Back on 21st of January]]

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.

Comment 9

[Daniel Stenberg [:bagder]]

Agreed. Try run, which looks similar to the run before this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=452748a4620250fee36ff6fd7529d7b48499103d

Comment 10

[Patrick McManus [:mcmanus]]

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