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

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_

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

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

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

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.

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;
 }

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

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.