Fix use after free bug by calling GetTokenService in Core's ctor.

google_apis/gaia/oauth2_token_service_request.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "google_apis/gaia/oauth2_token_service_request.h"
 
 #include "base/bind.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 22 matching lines @@
 // 4. Stop() is called on owner thread, which calls StopOnTokenServiceThread()
 // on token service thread.
 //
 // 5. Core is destroyed on owner thread.
 class OAuth2TokenServiceRequest::Core
     : public base::NonThreadSafe,
       public base::RefCountedThreadSafe<OAuth2TokenServiceRequest::Core> {
  public:
   // Note the thread where an instance of Core is constructed is referred to as
   // the "owner thread" here.
-  Core(OAuth2TokenServiceRequest* owner, TokenServiceProvider* provider);
+  Core(OAuth2TokenServiceRequest* owner,
+       const scoped_refptr<TokenServiceProvider>& provider);
 
   // Starts the core.  Must be called on the owner thread.
   void Start();
 
   // Stops the core.  Must be called on the owner thread.
   void Stop();
 
   // Returns true if this object has been stopped.  Must be called on the owner
   // thread.
   bool IsStopped() const;
@@ skipping 14 matching lines @@
   OAuth2TokenService* token_service();
   OAuth2TokenServiceRequest* owner();
 
  private:
   friend class base::RefCountedThreadSafe<OAuth2TokenServiceRequest::Core>;
 
   void DoNothing();
 
   scoped_refptr<base::SingleThreadTaskRunner> token_service_task_runner_;
   OAuth2TokenServiceRequest* owner_;
-  TokenServiceProvider* provider_;
+
+  // It is important that provider_ is destroyed on the owner thread, not the

[blundell, 2014/08/13 06:58:05]

This comment is puzzling. Since |provider_| is a ref-counted object that is
passed to this object from the outside, we actually don't have control over the
destruction of |provider_|.

[maniscalco, 2014/08/13 16:13:03]

You're right, this comment is incorrect.  The class comment for TSP is related
and was also incorrect until Roger pointed it out and I fixed it.  Looks like I
missed this one.

The idea is that we don't want a future maintainer to cause provider_ to be
destroyed on any thread other than the owner thread.  OAuth2TokenServiceRequest
is promising to clear references to TSP only on the owner thread.

Why does OA2TSP care about not causing TSP to be destroyed on a thread other
than the owner thread?  Only because it promised that in the TSP class comment.

Happy to fix this comment in a follow up CL.  Open to suggestions on how to word
it clearly.

+  // token_service_task_runner_ thread.
+  scoped_refptr<TokenServiceProvider> provider_;
+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
-OAuth2TokenServiceRequest::Core::Core(OAuth2TokenServiceRequest* owner,
-                                      TokenServiceProvider* provider)
+OAuth2TokenServiceRequest::Core::Core(
+    OAuth2TokenServiceRequest* owner,
+    const scoped_refptr<TokenServiceProvider>& provider)
     : owner_(owner), provider_(provider) {
   DCHECK(owner_);
   DCHECK(provider_);
   token_service_task_runner_ = provider_->GetTokenServiceTaskRunner();
   DCHECK(token_service_task_runner_);
 }
 
 OAuth2TokenServiceRequest::Core::~Core() {
 }
 
@@ skipping 48 matching lines @@
   DCHECK(CalledOnValidThread());
 }
 
 namespace {
 
 // An implementation of Core for getting an access token.
 class RequestCore : public OAuth2TokenServiceRequest::Core,
                     public OAuth2TokenService::Consumer {
  public:
   RequestCore(OAuth2TokenServiceRequest* owner,
-              OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+              const scoped_refptr<
+                  OAuth2TokenServiceRequest::TokenServiceProvider>& provider,
               OAuth2TokenService::Consumer* consumer,
               const std::string& account_id,
               const OAuth2TokenService::ScopeSet& scopes);
 
   // OAuth2TokenService::Consumer.  Must be called on the token service thread.
   virtual void OnGetTokenSuccess(const OAuth2TokenService::Request* request,
                                  const std::string& access_token,
                                  const base::Time& expiration_time) OVERRIDE;
   virtual void OnGetTokenFailure(const OAuth2TokenService::Request* request,
                                  const GoogleServiceAuthError& error) OVERRIDE;
@@ skipping 19 matching lines @@
 
   // OAuth2TokenService request for fetching OAuth2 access token; it should be
   // created, reset and accessed only on the token service thread.
   scoped_ptr<OAuth2TokenService::Request> request_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestCore);
 };
 
 RequestCore::RequestCore(
     OAuth2TokenServiceRequest* owner,
-    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
+        provider,
     OAuth2TokenService::Consumer* consumer,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes)
     : OAuth2TokenServiceRequest::Core(owner, provider),
       OAuth2TokenService::Consumer("oauth2_token_service"),
       owner_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       consumer_(consumer),
       account_id_(account_id),
       scopes_(scopes) {
   DCHECK(consumer_);
@@ skipping 50 matching lines @@
   DCHECK(CalledOnValidThread());
   if (!IsStopped()) {
     consumer_->OnGetTokenFailure(owner(), error);
   }
 }
 
 // An implementation of Core for invalidating an access token.
 class InvalidateCore : public OAuth2TokenServiceRequest::Core {
  public:
   InvalidateCore(OAuth2TokenServiceRequest* owner,
-                 OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+                 const scoped_refptr<
+                     OAuth2TokenServiceRequest::TokenServiceProvider>& provider,
                  const std::string& access_token,
                  const std::string& account_id,
                  const OAuth2TokenService::ScopeSet& scopes);
 
  private:
   friend class base::RefCountedThreadSafe<InvalidateCore>;
 
   // Must be destroyed on the owner thread.
   virtual ~InvalidateCore();
 
   // Core implementation.
   virtual void StartOnTokenServiceThread() OVERRIDE;
   virtual void StopOnTokenServiceThread() OVERRIDE;
 
   std::string access_token_;
   std::string account_id_;
   OAuth2TokenService::ScopeSet scopes_;
 
   DISALLOW_COPY_AND_ASSIGN(InvalidateCore);
 };
 
 InvalidateCore::InvalidateCore(
     OAuth2TokenServiceRequest* owner,
-    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>&
+        provider,
     const std::string& access_token,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes)
     : OAuth2TokenServiceRequest::Core(owner, provider),
       access_token_(access_token),
       account_id_(account_id),
       scopes_(scopes) {
   DCHECK(!access_token_.empty());
   DCHECK(!account_id_.empty());
   DCHECK(!scopes.empty());
 }
 
 InvalidateCore::~InvalidateCore() {
 }
 
 void InvalidateCore::StartOnTokenServiceThread() {
   DCHECK(token_service_task_runner()->BelongsToCurrentThread());
   token_service()->InvalidateToken(account_id_, scopes_, access_token_);
 }
 
 void InvalidateCore::StopOnTokenServiceThread() {
   DCHECK(token_service_task_runner()->BelongsToCurrentThread());
   // Nothing to do.
 }
 
 }  // namespace
 
 // static
 scoped_ptr<OAuth2TokenServiceRequest> OAuth2TokenServiceRequest::CreateAndStart(
-    TokenServiceProvider* provider,
+    const scoped_refptr<TokenServiceProvider>& provider,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes,
     OAuth2TokenService::Consumer* consumer) {
   scoped_ptr<OAuth2TokenServiceRequest> request(
       new OAuth2TokenServiceRequest(account_id));
   scoped_refptr<Core> core(
       new RequestCore(request.get(), provider, consumer, account_id, scopes));
   request->StartWithCore(core);
   return request.Pass();
 }
 
 // static
 void OAuth2TokenServiceRequest::InvalidateToken(
-    OAuth2TokenServiceRequest::TokenServiceProvider* provider,
+    const scoped_refptr<TokenServiceProvider>& provider,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes,
     const std::string& access_token) {
   scoped_ptr<OAuth2TokenServiceRequest> request(
       new OAuth2TokenServiceRequest(account_id));
   scoped_refptr<Core> core(new InvalidateCore(
       request.get(), provider, access_token, account_id, scopes));
   request->StartWithCore(core);
 }
 
 OAuth2TokenServiceRequest::~OAuth2TokenServiceRequest() {
   core_->Stop();
 }
 
 std::string OAuth2TokenServiceRequest::GetAccountId() const {
   return account_id_;
 }
 
 OAuth2TokenServiceRequest::OAuth2TokenServiceRequest(
     const std::string& account_id)
     : account_id_(account_id) {
   DCHECK(!account_id_.empty());
 }
 
 void OAuth2TokenServiceRequest::StartWithCore(const scoped_refptr<Core>& core) {
   DCHECK(core);
   core_ = core;
   core_->Start();
 }