Fix the case where the browser livelocks if we cannot open a file.

If one tries to upload a file that one doesn't have read access to,
the browser livelocks. It tries to read from the file, gets nothing
but spins forever because it knows that it hasn't finished reading.

To address this, firstly we add a check at stat() time to make sure
that we can read the file. However, this doesn't take care of the case
where the access() call was incorrect, or the permissions have changed
under us. In this case, we replace the missing file with NULs.

(Land attempt two: first in r39446, reverted in r39448)

http://codereview.chromium.org/541022
BUG=30850
TEST=Try to upload a file that isn't readable (i.e. /etc/shadow). The resulting upload should be a 0 byte file.

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

10 files changed, 213 insertions, 65 deletions

diff --git a/base/platform_file.h b/base/platform_file.h
index 0dbf4e4..46ff0cc 100644
--- a/base/platform_file.h
+++ b/base/platform_file.h
@@ -6,6 +6,8 @@
 #define BASE_PLATFORM_FILE_H_
 
 #include "build/build_config.h"
+#include "base/basictypes.h"
+
 #if defined(OS_WIN)
 #include <windows.h>
 #endif
@@ -53,6 +55,10 @@ PlatformFile CreatePlatformFile(const std::wstring& name,
 // Closes a file handle
 bool ClosePlatformFile(PlatformFile file);
 
+// Get the length of an underlying file. Returns false on error. Otherwise
+// *size is set to the length of the file, in bytes.
+bool GetPlatformFileSize(PlatformFile file, uint64* size);
+
 }  // namespace base
 
 #endif  // BASE_PLATFORM_FILE_H_
diff --git a/base/platform_file_posix.cc b/base/platform_file_posix.cc
index 46039b9..bfd40e9 100644
--- a/base/platform_file_posix.cc
+++ b/base/platform_file_posix.cc
@@ -77,4 +77,12 @@ bool ClosePlatformFile(PlatformFile file) {
   return close(file);
 }
 
+bool GetPlatformFileSize(PlatformFile file, uint64* out_size) {
+  struct stat st;
+  if (fstat(file, &st))
+    return false;
+  *out_size = st.st_size;
+  return true;
+}
+
 }  // namespace base
diff --git a/base/platform_file_win.cc b/base/platform_file_win.cc
index 1143487..ccaee1e 100644
--- a/base/platform_file_win.cc
+++ b/base/platform_file_win.cc
@@ -75,4 +75,12 @@ bool ClosePlatformFile(PlatformFile file) {
   return (CloseHandle(file) == 0);
 }
 
+bool GetPlatformFileSize(PlatformFile file, uint64* out_size) {
+  LARGE_INTEGER size;
+  if (!GetFileSizeEx(file, &size))
+    return false;
+  *out_size = size.QuadPart;
+  return true;
+}
+
 }  // namespace disk_cache
diff --git a/net/base/file_stream.h b/net/base/file_stream.h
index 6b7f3dc..def87bc 100644
--- a/net/base/file_stream.h
+++ b/net/base/file_stream.h
@@ -43,13 +43,24 @@ class FileStream {
   // Note that if there are any pending async operations, they'll be aborted.
   void Close();
 
+  // Release performs the same actions as Close, but doesn't actually close the
+  // underlying PlatformFile.
+  void Release();
+
   // Call this method to open the FileStream.  The remaining methods
   // cannot be used unless this method returns OK.  If the file cannot be
   // opened then an error code is returned.
   // open_flags is a bitfield of base::PlatformFileFlags
   int Open(const FilePath& path, int open_flags);
 
-  // Returns true if Open succeeded and Close has not been called.
+  // Calling this method is functionally the same as constructing the object
+  // with the non-default constructor. This method can only be used if the
+  // FileSteam isn't currently open (i.e. was constructed with the default
+  // constructor).
+  int Open(base::PlatformFile file, int open_flags);
+
+  // Returns true if Open succeeded and neither Close nor Release have been
+  // called.
   bool IsOpen() const;
 
   // Adjust the position from where data is read.  Upon success, the stream
diff --git a/net/base/file_stream_posix.cc b/net/base/file_stream_posix.cc
index a4c5b3c..e947a6e 100644
--- a/net/base/file_stream_posix.cc
+++ b/net/base/file_stream_posix.cc
@@ -298,13 +298,8 @@ FileStream::FileStream()
 }
 
 FileStream::FileStream(base::PlatformFile file, int flags)
-    : file_(file),
-      open_flags_(flags) {
-  // If the file handle is opened with base::PLATFORM_FILE_ASYNC, we need to
-  // make sure we will perform asynchronous File IO to it.
-  if (flags & base::PLATFORM_FILE_ASYNC) {
-    async_context_.reset(new AsyncContext());
-  }
+    : file_(base::kInvalidPlatformFileValue) {
+  Open(file, flags);
 }
 
 FileStream::~FileStream() {
@@ -323,6 +318,12 @@ void FileStream::Close() {
   }
 }
 
+void FileStream::Release() {
+  // Abort any existing asynchronous operations.
+  async_context_.reset();
+  file_ = base::kInvalidPlatformFileValue;
+}
+
 int FileStream::Open(const FilePath& path, int open_flags) {
   if (IsOpen()) {
     DLOG(FATAL) << "File is already open!";
@@ -344,6 +345,21 @@ int FileStream::Open(const FilePath& path, int open_flags) {
   return OK;
 }
 
+int FileStream::Open(base::PlatformFile file, int open_flags) {
+  if (IsOpen()) {
+    DLOG(FATAL) << "File is already open!";
+    return ERR_UNEXPECTED;
+  }
+
+  open_flags_ = open_flags;
+  file_ = file;
+
+  if (open_flags & base::PLATFORM_FILE_ASYNC)
+    async_context_.reset(new AsyncContext());
+
+  return OK;
+}
+
 bool FileStream::IsOpen() const {
   return file_ != base::kInvalidPlatformFileValue;
 }
@@ -445,7 +461,7 @@ int64 FileStream::Truncate(int64 bytes) {
   if (!IsOpen())
     return ERR_UNEXPECTED;
 
-  // We better be open for reading.
+  // We better be open for writing.
   DCHECK(open_flags_ & base::PLATFORM_FILE_WRITE);
 
   // Seek to the position to truncate from.
diff --git a/net/base/file_stream_win.cc b/net/base/file_stream_win.cc
index cec6a9d..e1928a7 100644
--- a/net/base/file_stream_win.cc
+++ b/net/base/file_stream_win.cc
@@ -120,16 +120,10 @@ FileStream::FileStream()
       open_flags_(0) {
 }
 
-FileStream::FileStream(base::PlatformFile file, int flags)
-    : file_(file),
-      open_flags_(flags) {
-  // If the file handle is opened with base::PLATFORM_FILE_ASYNC, we need to
-  // make sure we will perform asynchronous File IO to it.
-  if (flags & base::PLATFORM_FILE_ASYNC) {
-    async_context_.reset(new AsyncContext(this));
-    MessageLoopForIO::current()->RegisterIOHandler(file_,
-                                                   async_context_.get());
-  }
+FileStream::FileStream(base::PlatformFile file, int open_flags)
+    : file_(INVALID_HANDLE_VALUE),
+      open_flags_(0) {
+  Open(file, open_flags);
 }
 
 FileStream::~FileStream() {
@@ -147,6 +141,13 @@ void FileStream::Close() {
   }
 }
 
+void FileStream::Release() {
+  if (file_ != INVALID_HANDLE_VALUE)
+    CancelIo(file_);
+  async_context_.reset();
+  file_ = INVALID_HANDLE_VALUE;
+}
+
 int FileStream::Open(const FilePath& path, int open_flags) {
   if (IsOpen()) {
     DLOG(FATAL) << "File is already open!";
@@ -170,6 +171,26 @@ int FileStream::Open(const FilePath& path, int open_flags) {
   return OK;
 }
 
+int FileStream::Open(base::PlatformFile file, int open_flags) {
+  if (IsOpen()) {
+    DLOG(FATAL) << "File is already open!";
+    return ERR_UNEXPECTED;
+  }
+
+  open_flags_ = open_flags;
+  file_ = file;
+
+  // If the file handle is opened with base::PLATFORM_FILE_ASYNC, we need to
+  // make sure we will perform asynchronous File IO to it.
+  if (open_flags_ & base::PLATFORM_FILE_ASYNC) {
+    async_context_.reset(new AsyncContext(this));
+    MessageLoopForIO::current()->RegisterIOHandler(file_,
+                                                   async_context_.get());
+  }
+
+  return OK;
+}
+
 bool FileStream::IsOpen() const {
   return file_ != INVALID_HANDLE_VALUE;
 }
diff --git a/net/base/upload_data.cc b/net/base/upload_data.cc
index 0496873..a23b25e 100644
--- a/net/base/upload_data.cc
+++ b/net/base/upload_data.cc
@@ -5,6 +5,7 @@
 #include "net/base/upload_data.h"
 
 #include "base/file_util.h"
+#include "base/platform_file.h"
 #include "base/logging.h"
 
 namespace net {
@@ -17,28 +18,67 @@ uint64 UploadData::GetContentLength() const {
   return len;
 }
 
-uint64 UploadData::Element::GetContentLength() const {
-  if (override_content_length_)
-    return content_length_;
+void UploadData::CloseFiles() {
+  std::vector<Element>::iterator it = elements_.begin();
+  for (; it != elements_.end(); ++it) {
+    if (it->type() == TYPE_FILE)
+      it->Close();
+  }
+}
+
+base::PlatformFile UploadData::Element::platform_file() const {
+  DCHECK(type_ == TYPE_FILE) << "platform_file on non-file Element";
+
+  return file_;
+}
+
+void UploadData::Element::Close() {
+  DCHECK(type_ == TYPE_FILE) << "Close on non-file Element";
+
+  if (file_ != base::kInvalidPlatformFileValue)
+    base::ClosePlatformFile(file_);
+  file_ = base::kInvalidPlatformFileValue;
+}
+
+void UploadData::Element::SetToFilePathRange(const FilePath& path,
+                                             uint64 offset,
+                                             uint64 length) {
+  type_ = TYPE_FILE;
+  file_range_offset_ = 0;
+  file_range_length_ = 0;
 
-  if (type_ == TYPE_BYTES)
-    return static_cast<uint64>(bytes_.size());
+  Close();
 
-  DCHECK(type_ == TYPE_FILE);
+  if (offset + length < offset) {
+    LOG(ERROR) << "Upload file offset and length overflow 64-bits. Ignoring.";
+    return;
+  }
 
-  // TODO(darin): This size calculation could be out of sync with the state of
-  // the file when we get around to reading it.  We should probably find a way
-  // to lock the file or somehow protect against this error condition.
+  base::PlatformFile file = base::CreatePlatformFile(
+      path, base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, NULL);
+  if (file == base::kInvalidPlatformFileValue) {
+    // This case occurs when the user selects a file that isn't readable.
+    file_path_= path;
+    return;
+  }
 
-  int64 length = 0;
-  if (!file_util::GetFileSize(file_path_, &length))
-    return 0;
+  uint64 file_size;
+  if (!base::GetPlatformFileSize(file, &file_size)) {
+    base::ClosePlatformFile(file);
+    return;
+  }
 
-  if (file_range_offset_ >= static_cast<uint64>(length))
-    return 0;  // range is beyond eof
+  if (offset > file_size) {
+    base::ClosePlatformFile(file);
+    return;
+  }
+  if (offset + length > file_size)
+    length = file_size - offset;
 
-  // compensate for the offset and clip file_range_length_ to eof
-  return std::min(length - file_range_offset_, file_range_length_);
+  file_ = file;
+  file_path_ = path;
+  file_range_offset_ = offset;
+  file_range_length_ = length;
 }
 
 }  // namespace net
diff --git a/net/base/upload_data.h b/net/base/upload_data.h
index 3aab835..d01b435 100644
--- a/net/base/upload_data.h
+++ b/net/base/upload_data.h
@@ -9,6 +9,7 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
+#include "base/platform_file.h"
 #include "base/ref_counted.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
@@ -26,7 +27,8 @@ class UploadData : public base::RefCounted<UploadData> {
   class Element {
    public:
     Element() : type_(TYPE_BYTES), file_range_offset_(0),
-                file_range_length_(kuint64max),
+                file_range_length_(0),
+                file_(base::kInvalidPlatformFileValue),
                 override_content_length_(false) {
     }
 
@@ -45,19 +47,42 @@ class UploadData : public base::RefCounted<UploadData> {
       SetToFilePathRange(path, 0, kuint64max);
     }
 
-    void SetToFilePathRange(const FilePath& path,
-                            uint64 offset, uint64 length) {
-      type_ = TYPE_FILE;
-      file_path_ = path;
-      file_range_offset_ = offset;
-      file_range_length_ = length;
-    }
+    void SetToFilePathRange(const FilePath& path, uint64 offset, uint64 length);
 
     // Returns the byte-length of the element.  For files that do not exist, 0
     // is returned.  This is done for consistency with Mozilla.
-    uint64 GetContentLength() const;
+    uint64 GetContentLength() const {
+      if (override_content_length_)
+        return content_length_;
+
+      if (type_ == TYPE_BYTES) {
+        return bytes_.size();
+      } else {
+        return file_range_length_;
+      }
+    }
+
+    // For a TYPE_FILE, return a handle to the file. The caller does not take
+    // ownership and should not close the file handle.
+    base::PlatformFile platform_file() const;
+
+    // For a TYPE_FILE, this closes the file handle. It's a fatal error to call
+    // platform_file() after this.
+    void Close();
 
    private:
+    // type_ == TYPE_BYTES:
+    //   bytes_ is valid
+    // type_ == TYPE_FILE:
+    //   file_path_ should always be valid.
+    //
+    //   platform_file() may be invalid, in which case file_range_* are 0 and
+    //   file_ is invalid. This occurs when we cannot open the requested file.
+    //
+    //   Else, then file_range_* are within range of the length of the file
+    //   that we found when opening the file. Also, the sum of offset and
+    //   length will not overflow a uint64. file_ will be handle to the file.
+
     // Allows tests to override the result of GetContentLength.
     void SetContentLength(uint64 content_length) {
       override_content_length_ = true;
@@ -69,6 +94,7 @@ class UploadData : public base::RefCounted<UploadData> {
     FilePath file_path_;
     uint64 file_range_offset_;
     uint64 file_range_length_;
+    base::PlatformFile file_;
     bool override_content_length_;
     uint64 content_length_;
 
@@ -109,6 +135,9 @@ class UploadData : public base::RefCounted<UploadData> {
     elements_.swap(*elements);
   }
 
+  // CloseFiles closes the file handles of all Elements of type TYPE_FILE.
+  void CloseFiles();
+
   // Identifies a particular upload instance, which is used by the cache to
   // formulate a cache key.  This value should be unique across browser
   // sessions.  A value of 0 is used to indicate an unspecified identifier.
diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index 221ef28..0a0ee73 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -10,7 +10,7 @@
 
 namespace net {
 
-UploadDataStream::UploadDataStream(const UploadData* data)
+UploadDataStream::UploadDataStream(UploadData* data)
     : data_(data),
       buf_(new IOBuffer(kBufSize)),
       buf_len_(0),
@@ -24,6 +24,7 @@ UploadDataStream::UploadDataStream(const UploadData* data)
 }
 
 UploadDataStream::~UploadDataStream() {
+  data_->CloseFiles();
 }
 
 void UploadDataStream::DidConsume(size_t num_bytes) {
@@ -67,18 +68,19 @@ void UploadDataStream::FillBuf() {
     } else {
       DCHECK(element.type() == UploadData::TYPE_FILE);
 
-      if (!next_element_stream_.IsOpen()) {
-        int flags = base::PLATFORM_FILE_OPEN |
-                    base::PLATFORM_FILE_READ;
-        int rv = next_element_stream_.Open(element.file_path(), flags);
-        // If the file does not exist, that's technically okay.. we'll just
-        // upload an empty file.  This is for consistency with Mozilla.
-        DLOG_IF(WARNING, rv != OK) << "Failed to open \""
-                                   << element.file_path().value()
-                                   << "\" for reading: " << rv;
-
-        next_element_remaining_ = 0;  // Default to reading nothing.
-        if (rv == OK) {
+      if (element.file_range_length() == 0) {
+        // If we failed to open the file, then the length is set to zero. The
+        // length used when calculating the POST size was also zero. This
+        // matches the behaviour of Mozilla.
+        next_element_remaining_ = 0;
+      } else {
+        if (!next_element_stream_.IsOpen()) {
+          // We ignore the return value of Open becuase we've already checked
+          // !IsOpen, above.
+          int flags = base::PLATFORM_FILE_READ |
+                      base::PLATFORM_FILE_WRITE;
+          next_element_stream_.Open(element.platform_file(), flags);
+
           uint64 offset = element.file_range_offset();
           if (offset && next_element_stream_.Seek(FROM_BEGIN, offset) < 0) {
             DLOG(WARNING) << "Failed to seek \"" << element.file_path().value()
@@ -90,11 +92,18 @@ void UploadDataStream::FillBuf() {
       }
 
       int rv = 0;
-      int count = static_cast<int>(std::min(
-          static_cast<uint64>(size_remaining), next_element_remaining_));
-      if (count > 0 &&
-          (rv = next_element_stream_.Read(buf_->data() + buf_len_,
-                                          count, NULL)) > 0) {
+      if (next_element_remaining_ > 0) {
+        int count =
+            static_cast<int>(std::min(next_element_remaining_,
+                                      static_cast<uint64>(size_remaining)));
+        rv = next_element_stream_.Read(buf_->data() + buf_len_, count, NULL);
+        if (rv < 1) {
+          // If the file was truncated between the time that we opened it and
+          // now, or if we got an error on reading, then we pad with NULs.
+          memset(buf_->data() + buf_len_, 0, count);
+          rv = count;
+        }
+
         buf_len_ += rv;
         next_element_remaining_ -= rv;
       } else {
@@ -105,7 +114,7 @@ void UploadDataStream::FillBuf() {
     if (advance_to_next_element) {
       ++next_element_;
       next_element_offset_ = 0;
-      next_element_stream_.Close();
+      next_element_stream_.Release();
     }
   }
 
diff --git a/net/base/upload_data_stream.h b/net/base/upload_data_stream.h
index 0dd7dd3..9df5db0 100644
--- a/net/base/upload_data_stream.h
+++ b/net/base/upload_data_stream.h
@@ -14,7 +14,7 @@ class IOBuffer;
 
 class UploadDataStream {
  public:
-  explicit UploadDataStream(const UploadData* data);
+  explicit UploadDataStream(UploadData* data);
   ~UploadDataStream();
 
   // Returns the stream's buffer and buffer length.
@@ -42,7 +42,7 @@ class UploadDataStream {
   // left to fill the buffer with.
   void FillBuf();
 
-  const UploadData* data_;
+  UploadData* data_;
 
   // This buffer is filled with data to be uploaded.  The data to be sent is
   // always at the front of the buffer.  If we cannot send all of the buffer at