Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers.

components/cronet/android/url_request_adapter.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 "components/cronet/android/url_request_adapter.h"
 
 #include <stddef.h>
 #include <string.h>
 #include <utility>
 
@@ skipping 112 matching lines @@
   context_->PostTaskToNetworkThread(
       FROM_HERE,
       base::Bind(&URLRequestAdapter::OnInitiateConnection,
                  base::Unretained(this)));
 }
 
 void URLRequestAdapter::OnAppendChunk(const scoped_ptr<char[]> bytes,
                                       int bytes_len, bool is_last_chunk) {
   DCHECK(OnNetworkThread());
   // Request could have completed and been destroyed on the network thread
   // while appendChunk was posting the task from an application thread.

[eroman, 2016/03/22 19:05:37]

This comment and subsequent VLOG() don't seem congruent with the new condition.

[mmenke, 2016/03/22 20:01:50]

Comment seems correct - you can actually succeed without completing the upload
(With, for instance, a 413 - we check for a response after the connection has
been closed for just that case), but it's uncommon.

I've restated the comment, and removed the VLOG - I don't think it's that
useful.

-  if (!url_request_) {
+  if (!chunked_upload_writer_->AppendData(bytes.get(), bytes_len,
+                                          is_last_chunk)) {
     VLOG(1) << "Cannot append chunk to destroyed request: "
             << url_.possibly_invalid_spec().c_str();
     return;
   }
-  url_request_->AppendChunkToUpload(bytes.get(), bytes_len, is_last_chunk);
 }
 
 void URLRequestAdapter::OnInitiateConnection() {
   DCHECK(OnNetworkThread());
   if (canceled_) {
     return;
   }
 
   VLOG(1) << "Starting chromium request: "
           << url_.possibly_invalid_spec().c_str()
           << " priority: " << RequestPriorityToString(priority_);
   url_request_ = context_->GetURLRequestContext()->CreateRequest(
       url_, net::DEFAULT_PRIORITY, this);
   int flags = net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES;
   if (context_->load_disable_cache())
     flags |= net::LOAD_DISABLE_CACHE;
   url_request_->SetLoadFlags(flags);
   url_request_->set_method(method_);
   url_request_->SetExtraRequestHeaders(headers_);
   if (!headers_.HasHeader(net::HttpRequestHeaders::kUserAgent)) {
     std::string user_agent;
     user_agent = context_->GetUserAgent(url_);
     url_request_->SetExtraRequestHeaderByName(
         net::HttpRequestHeaders::kUserAgent, user_agent, true /* override */);
   }
 
   if (upload_data_stream_) {
     url_request_->set_upload(std::move(upload_data_stream_));
   } else if (chunked_upload_) {
-    url_request_->EnableChunkedUpload();
+    scoped_ptr<net::ChunkedUploadDataStream> chunked_upload_data_stream(
+        new net::ChunkedUploadDataStream(0));
+    chunked_upload_writer_ = chunked_upload_data_stream->CreateWriter();
+    url_request_->set_upload(std::move(chunked_upload_data_stream));

[eroman, 2016/03/22 19:05:37]

This looked wrong when first reading it because of the apparent reference cycle.

I figured the writer would be holding a weak ptr, but worth adding a comment
IMO.

[mmenke, 2016/03/22 20:01:50]

Done.

   }
 
   url_request_->SetPriority(priority_);
 
   url_request_->Start();
 }
 
 void URLRequestAdapter::Cancel() {
   context_->PostTaskToNetworkThread(
       FROM_HERE,
@@ skipping 141 matching lines @@
 unsigned char* URLRequestAdapter::Data() const {
   DCHECK(OnNetworkThread());
   return reinterpret_cast<unsigned char*>(read_buffer_->data());
 }
 
 bool URLRequestAdapter::OnNetworkThread() const {
   return context_->GetNetworkTaskRunner()->BelongsToCurrentThread();
 }
 
 }  // namespace cronet