Prevent a crash that can occur when redirecting a file URL.

We redirect a file URL that corresponds to a directory when the file URL does
not end in a slash.  We redirect to the same URL with a slash appended to
simplify other code, which can then rely on the presence of a trailing slash
for all file URLs that correspond to a directory.

It turns out that following the redirect could result in the job being used
after free.  The code in URLRequestJob::FollowDeferredRedirect clears some
fields after calling FollowRedirect.  For all other jobs this is not a problem
since the process of following a redirect causes the job to be killed, and
URLRequestJob::Kill takes a reference to the job.  It turns out that
URLRequestFileDirJob was not calling URLRequestJob::Kill, and so this extra
reference was not being taken.

The fix is two-fold:

1-  Make URLRequestFileDirJob call URLRequestJob::Kill just like the rest of
the jobs.  This seems like a good idea for other reasons as well.

2-  Make URLRequestJob::FollowDeferredRedirect not depend on itself being alive
after the FollowRedirect call.  This just seems good for future cases where a
special URLRequestJob subclass might not call URLRequestJob::Kill for some
reason or another.

Finally, some minor changes were rqeuired to URLRequestFileDirJob to support
the call to Kill.  See changes to OnListDone.

Writing a unit test for this was a bit tricky.  It turns out that while the
URLRequestFileDirJob is waiting for the client to call FollowDeferredRedirect,
it is still traversing the directory.  It does not pause the directory listing.
The crash could only happen if the directory listing completed before the
consumer called FollowDeferredRedirect because a reference to the job was taken
on behalf of the DirectoryLister.  To test this condition, I made it possible
for a test to poll the URLRequestFileDirJob::list_completed_ flag.  Once that
is set, I then have the test call FollowDeferredRedirect.

NOTE: When running within net_unittests, NetModule::SetResourceProvider has not
been called.  So, I downgraded a NOTREACHED to a DLOG(WARNING).  NOTREACHED was
a bit excessive since it is a condition that can be reached, and I don't see
any problem with allowing unit tests to function without a resource provider.

R=wtc
BUG=18686
TEST=URLRequestTest.FileDirRedirectNoCrash

Review URL: http://codereview.chromium.org/174076

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@23944 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 65 insertions, 10 deletions

diff --git a/net/base/net_util.cc b/net/base/net_util.cc
index 11941a7..d679e14 100644
--- a/net/base/net_util.cc
+++ b/net/base/net_util.cc
@@ -912,10 +912,13 @@ std::string CanonicalizeHost(const std::wstring& host,
 
 std::string GetDirectoryListingHeader(const string16& title) {
   static const StringPiece header(NetModule::GetResource(IDR_DIR_HEADER_HTML));
-  if (header.empty()) {
-    NOTREACHED() << "expected resource not found";
-  }
-  std::string result(header.data(), header.size());
+  // This can be null in unit tests.
+  DLOG_IF(WARNING, header.empty()) <<
+      "Missing resource: directory listing header";
+
+  std::string result;
+  if (!header.empty())
+    result.assign(header.data(), header.size());
 
   result.append("<script>start(");
   string_escape::JsonDoubleQuote(title, true, &result);
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index aa76b00..9f0f500 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -357,6 +357,7 @@ void URLRequest::ResponseStarted() {
 
 void URLRequest::FollowDeferredRedirect() {
   DCHECK(job_);
+  DCHECK(status_.is_success());
 
   job_->FollowDeferredRedirect();
 }
diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h
index 00ff344..bdd76e7 100644
--- a/net/url_request/url_request.h
+++ b/net/url_request/url_request.h
@@ -499,6 +499,10 @@ class URLRequest {
     priority_ = priority;
   }
 
+#ifdef UNIT_TEST
+  URLRequestJob* job() { return job_; }
+#endif
+
  protected:
   // Allow the URLRequestJob class to control the is_pending() flag.
   void set_is_pending(bool value) { is_pending_ = value; }
diff --git a/net/url_request/url_request_file_dir_job.cc b/net/url_request/url_request_file_dir_job.cc
index ecdf014..a933274 100644
--- a/net/url_request/url_request_file_dir_job.cc
+++ b/net/url_request/url_request_file_dir_job.cc
@@ -67,6 +67,8 @@ void URLRequestFileDirJob::Kill() {
   // OnListDone will notify the URLRequest at this time.
   if (lister_)
     lister_->Cancel();
+
+  URLRequestJob::Kill();
 }
 
 bool URLRequestFileDirJob::ReadRawData(net::IOBuffer* buf, int buf_size,
@@ -148,12 +150,12 @@ void URLRequestFileDirJob::OnListFile(
 void URLRequestFileDirJob::OnListDone(int error) {
   CloseLister();
 
-  if (error) {
+  if (canceled_) {
     read_pending_ = false;
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, error));
-  } else if (canceled_) {
+    // No need for NotifyCanceled() since canceled_ is set inside Kill().
+  } else if (error) {
     read_pending_ = false;
-    NotifyCanceled();
+    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, error));
   } else {
     list_complete_ = true;
     CompleteRead();
diff --git a/net/url_request/url_request_file_dir_job.h b/net/url_request/url_request_file_dir_job.h
index 606e3b5..52a6d61 100644
--- a/net/url_request/url_request_file_dir_job.h
+++ b/net/url_request/url_request_file_dir_job.h
@@ -32,6 +32,8 @@ class URLRequestFileDirJob
   virtual void OnListFile(const file_util::FileEnumerator::FindInfo& data);
   virtual void OnListDone(int error);
 
+  bool list_complete() const { return list_complete_; }
+
  private:
   void CloseLister();
   // When we have data and a read has been pending, this function
diff --git a/net/url_request/url_request_job.cc b/net/url_request/url_request_job.cc
index 4518d8a..b84fdd5 100644
--- a/net/url_request/url_request_job.cc
+++ b/net/url_request/url_request_job.cc
@@ -122,13 +122,21 @@ void URLRequestJob::ContinueDespiteLastError() {
 
 void URLRequestJob::FollowDeferredRedirect() {
   DCHECK(deferred_redirect_status_code_ != -1);
+
   // NOTE: deferred_redirect_url_ may be invalid, and attempting to redirect to
   // such an URL will fail inside FollowRedirect.  The DCHECK above asserts
   // that we called OnReceivedRedirect.
 
-  FollowRedirect(deferred_redirect_url_, deferred_redirect_status_code_);
+  // It is also possible that FollowRedirect will drop the last reference to
+  // this job, so we need to reset our members before calling it.
+
+  GURL redirect_url = deferred_redirect_url_;
+  int redirect_status_code = deferred_redirect_status_code_;
+
   deferred_redirect_url_ = GURL();
   deferred_redirect_status_code_ = -1;
+
+  FollowRedirect(redirect_url, redirect_status_code);
 }
 
 int64 URLRequestJob::GetByteReadCount() const {
diff --git a/net/url_request/url_request_job.h b/net/url_request/url_request_job.h
index 60554ed..9a34f4a 100644
--- a/net/url_request/url_request_job.h
+++ b/net/url_request/url_request_job.h
@@ -64,7 +64,9 @@ class URLRequestJob : public base::RefCountedThreadSafe<URLRequestJob>,
 
   // This function MUST somehow call NotifyDone/NotifyCanceled or some requests
   // will get leaked. Certain callers use that message to know when they can
-  // delete their URLRequest object, even when doing a cancel.
+  // delete their URLRequest object, even when doing a cancel. The default Kill
+  // implementation calls NotifyCanceled, so it is recommended that subclasses
+  // call URLRequestJob::Kill() after doing any additional work.
   //
   // The job should endeavor to stop working as soon as is convenient, but must
   // not send and complete notifications from inside this function. Instead,
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index d68175b..afa2c6c 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -36,6 +36,7 @@
 #include "net/proxy/proxy_service.h"
 #include "net/socket/ssl_test_util.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_file_dir_job.h"
 #include "net/url_request/url_request_test_job.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
@@ -1011,6 +1012,38 @@ TEST_F(URLRequestTest, FileDirCancelTest) {
   net::NetModule::SetResourceProvider(NULL);
 }
 
+TEST_F(URLRequestTest, FileDirRedirectNoCrash) {
+  // There is an implicit redirect when loading a file path that matches a
+  // directory and does not end with a slash.  Ensure that following such
+  // redirects does not crash.  See http://crbug.com/18686.
+
+  FilePath path;
+  PathService::Get(base::DIR_SOURCE_ROOT, &path);
+  path = path.Append(FILE_PATH_LITERAL("net"));
+  path = path.Append(FILE_PATH_LITERAL("data"));
+  path = path.Append(FILE_PATH_LITERAL("url_request_unittest"));
+
+  TestDelegate d;
+  d.set_quit_on_redirect(true);
+  TestURLRequest req(net::FilePathToFileURL(path), &d);
+  req.Start();
+  MessageLoop::current()->Run();
+
+  // Let the directory lister have time to finish its work, which will
+  // cause the URLRequestFileDirJob's ref count to drop to 1.
+  URLRequestFileDirJob* job = static_cast<URLRequestFileDirJob*>(req.job());
+  while (!job->list_complete()) {
+    PlatformThread::Sleep(10);
+    MessageLoop::current()->RunAllPending();
+  }
+
+  // Should not crash during this call!
+  req.FollowDeferredRedirect();
+
+  // Flush event queue.
+  MessageLoop::current()->RunAllPending();
+}
+
 TEST_F(URLRequestTestHTTP, RestrictRedirects) {
   ASSERT_TRUE(NULL != server_.get());