Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers.

net/url_request/url_fetcher_core.cc

 // Copyright (c) 2012 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 "net/url_request/url_fetcher_core.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 187 matching lines @@
 
   upload_content_type_ = content_type;
   upload_content_.clear();
   is_chunked_upload_ = true;
 }
 
 void URLFetcherCore::AppendChunkToUpload(const std::string& content,
                                          bool is_last_chunk) {
   DCHECK(delegate_task_runner_.get());
   DCHECK(network_task_runner_.get());
+  DCHECK(is_chunked_upload_);
   network_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&URLFetcherCore::CompleteAddingUploadDataChunk, this, content,
                  is_last_chunk));
 }
 
 void URLFetcherCore::SetLoadFlags(int load_flags) {
   load_flags_ = load_flags;
 }
 
@@ skipping 255 matching lines @@
   const URLRequestStatus status = request_->status();
   if (status.is_success())
     request_->GetResponseCookies(&cookies_);
 
   // See comments re: HEAD requests in ReadResponse().
   if (!status.is_io_pending() || request_type_ == URLFetcher::HEAD) {
     status_ = status;
     received_response_content_length_ =
         request_->received_response_content_length();
     total_received_bytes_ += request_->GetTotalReceivedBytes();
     ReleaseRequest();

[mmenke, 2016/03/02 19:42:37]

Bug 2:  On error, we release the request without setting cancelled_ to true, so
the embedder may continue appending data before it knows about the failure, and
since cancelled_ isn't set to true,
URLFetcherCore::CompleteAddingUploadDataChunk will try to pass the data to the
deleted URLRequest.

 
     // No more data to write.
     const int result = response_writer_->Finish(
         base::Bind(&URLFetcherCore::DidFinishWriting, this));
     if (result != ERR_IO_PENDING)
       DidFinishWriting(result);
   }
 }
 
 void URLFetcherCore::OnContextShuttingDown() {
@@ skipping 15 matching lines @@
 
 URLFetcherCore::~URLFetcherCore() {
   // |request_| should be NULL. If not, it's unsafe to delete it here since we
   // may not be on the IO thread.
   DCHECK(!request_.get());
 }
 
 void URLFetcherCore::StartOnIOThread() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
+  // Create ChunkedUploadDataStream, if needed, so the consumer can start
+  // appending data.  Have to do it here because StartURLRequest() may be called
+  // asynchonously.
+  if (is_chunked_upload_) {
+    chunked_stream_.reset(new ChunkedUploadDataStream(0));
+    chunked_stream_writer_ = chunked_stream_->CreateWriter();
+  }
+
   if (!response_writer_)
     response_writer_.reset(new URLFetcherStringWriter);
 
   const int result = response_writer_->Initialize(
       base::Bind(&URLFetcherCore::DidInitializeWriter, this));
   if (result != ERR_IO_PENDING)
     DidInitializeWriter(result);

[mmenke, 2016/03/02 19:42:37]

Bug 1:  Note that this can call DidInitializeWriter asynchronously, which
creates the URLRequest.  If that happens, appending data before that method runs
will result in a crash.

 }
 
 void URLFetcherCore::StartURLRequest() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
   if (was_cancelled_) {
     // Since StartURLRequest() is posted as a *delayed* task, it may
     // run after the URLFetcher was already stopped.
     return;
   }
 
   if (!request_context_getter_->GetURLRequestContext()) {
     CancelRequestAndInformDelegate(ERR_CONTEXT_SHUT_DOWN);
     return;
   }
 
   DCHECK(request_context_getter_.get());
   DCHECK(!request_.get());
 
   g_registry.Get().AddURLFetcherCore(this);
   current_response_bytes_ = 0;
   request_context_getter_->AddObserver(this);
   request_ = request_context_getter_->GetURLRequestContext()->CreateRequest(
       original_url_, DEFAULT_PRIORITY, this);
   request_->set_stack_trace(stack_trace_);
   int flags = request_->load_flags() | load_flags_;
 
-  if (is_chunked_upload_)
-    request_->EnableChunkedUpload();
+  // TODO(mmenke): This should really be with the other code to set the upload
+  // body, below.
+  if (chunked_stream_)
+    request_->set_upload(make_scoped_ptr(chunked_stream_.release()));
+
   request_->SetLoadFlags(flags);
   request_->SetReferrer(referrer_);
   request_->set_referrer_policy(referrer_policy_);
   request_->set_first_party_for_cookies(initiator_.is_empty() ? original_url_
                                                               : initiator_);
   request_->set_initiator(initiator_.is_empty() ? url::Origin(original_url_)
                                                 : url::Origin(initiator_));
   if (url_request_data_key_ && !url_request_create_data_callback_.is_null()) {
     request_->SetUserData(url_request_data_key_,
                           url_request_create_data_callback_.Run());
@@ skipping 263 matching lines @@
     destination_url_backoff =
         url_throttler_entry_->GetExponentialBackoffReleaseTime();
   }
 
   return original_url_backoff > destination_url_backoff ?
       original_url_backoff : destination_url_backoff;
 }
 
 void URLFetcherCore::CompleteAddingUploadDataChunk(
     const std::string& content, bool is_last_chunk) {
-  if (was_cancelled_) {
-    // Since CompleteAddingUploadDataChunk() is posted as a *delayed* task, it
-    // may run after the URLFetcher was already stopped.
-    return;
-  }
   DCHECK(is_chunked_upload_);
-  DCHECK(request_.get());
   DCHECK(!content.empty());
-  request_->AppendChunkToUpload(content.data(),
-                                static_cast<int>(content.length()),
-                                is_last_chunk);
+  chunked_stream_writer_->AppendData(
+      content.data(), static_cast<int>(content.length()), is_last_chunk);
 }
 
 int URLFetcherCore::WriteBuffer(scoped_refptr<DrainableIOBuffer> data) {
   while (data->BytesRemaining() > 0) {
     const int result = response_writer_->Write(
         data.get(),
         data->BytesRemaining(),
         base::Bind(&URLFetcherCore::DidWriteBuffer, this, data));
     if (result < 0) {
       if (result != ERR_IO_PENDING)
@@ skipping 94 matching lines @@
 }
 
 void URLFetcherCore::AssertHasNoUploadData() const {
   DCHECK(!upload_content_set_);
   DCHECK(upload_content_.empty());
   DCHECK(upload_file_path_.empty());
   DCHECK(upload_stream_factory_.is_null());
 }
 
 }  // namespace net