diff options
| -rw-r--r-- | base/file_util.h | 54 | ||||
| -rw-r--r-- | chrome/browser/renderer_host/resource_dispatcher_host.cc | 4 | ||||
| -rw-r--r-- | chrome/common/common_param_traits.h | 2 | ||||
| -rw-r--r-- | net/base/upload_data.cc | 57 | ||||
| -rw-r--r-- | net/base/upload_data.h | 28 | ||||
| -rw-r--r-- | net/base/upload_data_stream.cc | 83 | ||||
| -rw-r--r-- | net/base/upload_data_stream.h | 10 | ||||
| -rw-r--r-- | net/base/upload_data_stream_unittest.cc | 4 | ||||
| -rw-r--r-- | net/http/http_network_transaction_unittest.cc | 149 |
9 files changed, 326 insertions, 65 deletions
diff --git a/base/file_util.h b/base/file_util.h index fdc13ee..2e509bb 100644 --- a/base/file_util.h +++ b/base/file_util.h @@ -12,6 +12,9 @@ #if defined(OS_WIN) #include <windows.h> +#if defined(UNIT_TEST) +#include <aclapi.h> +#endif #elif defined(OS_POSIX) #include <sys/stat.h> #endif @@ -502,6 +505,57 @@ bool RenameFileAndResetSecurityDescriptor( bool HasFileBeenModifiedSince(const FileEnumerator::FindInfo& find_info, const base::Time& cutoff_time); +#ifdef UNIT_TEST + +inline bool MakeFileUnreadable(const FilePath& path) { +#if defined(OS_POSIX) + struct stat stat_buf; + if (stat(path.value().c_str(), &stat_buf) != 0) + return false; + stat_buf.st_mode &= ~(S_IRUSR | S_IRGRP | S_IROTH); + + return chmod(path.value().c_str(), stat_buf.st_mode) == 0; + +#elif defined(OS_WIN) + PACL old_dacl; + PSECURITY_DESCRIPTOR security_descriptor; + if (GetNamedSecurityInfo(path.value().c_str(), SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, NULL, NULL, &old_dacl, + NULL, &security_descriptor) != ERROR_SUCCESS) + return false; + + // Deny Read access for the current user. + EXPLICIT_ACCESS change; + change.grfAccessPermissions = GENERIC_READ; + change.grfAccessMode = DENY_ACCESS; + change.grfInheritance = 0; + change.Trustee.pMultipleTrustee = NULL; + change.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; + change.Trustee.TrusteeForm = TRUSTEE_IS_NAME; + change.Trustee.TrusteeType = TRUSTEE_IS_USER; + change.Trustee.ptstrName = L"CURRENT_USER"; + + PACL new_dacl; + if (SetEntriesInAcl(1, &change, old_dacl, &new_dacl) != ERROR_SUCCESS) { + LocalFree(security_descriptor); + return false; + } + + DWORD rc = SetNamedSecurityInfo(const_cast<wchar_t*>(path.value().c_str()), + SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, + NULL, NULL, new_dacl, NULL); + LocalFree(security_descriptor); + LocalFree(new_dacl); + + return rc == ERROR_SUCCESS; +#else + NOTIMPLEMENTED(); + return false; +#endif +} + +#endif // UNIT_TEST + } // namespace file_util // Deprecated functions have been moved to this separate header file, diff --git a/chrome/browser/renderer_host/resource_dispatcher_host.cc b/chrome/browser/renderer_host/resource_dispatcher_host.cc index 143d969..7576926 100644 --- a/chrome/browser/renderer_host/resource_dispatcher_host.cc +++ b/chrome/browser/renderer_host/resource_dispatcher_host.cc @@ -132,10 +132,10 @@ bool ShouldServiceRequest(ChildProcessInfo::ProcessType process_type, // Check if the renderer is permitted to upload the requested files. if (request_data.upload_data) { - const std::vector<net::UploadData::Element>& uploads = + const std::vector<net::UploadData::Element>* uploads = request_data.upload_data->elements(); std::vector<net::UploadData::Element>::const_iterator iter; - for (iter = uploads.begin(); iter != uploads.end(); ++iter) { + for (iter = uploads->begin(); iter != uploads->end(); ++iter) { if (iter->type() == net::UploadData::TYPE_FILE && !policy->CanUploadFile(child_id, iter->file_path())) { NOTREACHED() << "Denied unauthorized upload of " diff --git a/chrome/common/common_param_traits.h b/chrome/common/common_param_traits.h index 426075a..2288f15 100644 --- a/chrome/common/common_param_traits.h +++ b/chrome/common/common_param_traits.h @@ -361,7 +361,7 @@ struct ParamTraits<scoped_refptr<net::UploadData> > { static void Write(Message* m, const param_type& p) { WriteParam(m, p.get() != NULL); if (p) { - WriteParam(m, p->elements()); + WriteParam(m, *p->elements()); WriteParam(m, p->identifier()); } } diff --git a/net/base/upload_data.cc b/net/base/upload_data.cc index 0496873..198b051 100644 --- a/net/base/upload_data.cc +++ b/net/base/upload_data.cc @@ -6,30 +6,42 @@ #include "base/file_util.h" #include "base/logging.h" +#include "net/base/net_errors.h" namespace net { -uint64 UploadData::GetContentLength() const { +uint64 UploadData::GetContentLength() { uint64 len = 0; - std::vector<Element>::const_iterator it = elements_.begin(); + std::vector<Element>::iterator it = elements_.begin(); for (; it != elements_.end(); ++it) len += (*it).GetContentLength(); return len; } -uint64 UploadData::Element::GetContentLength() const { - if (override_content_length_) +uint64 UploadData::Element::GetContentLength() { + if (override_content_length_ || content_length_computed_) return content_length_; if (type_ == TYPE_BYTES) return static_cast<uint64>(bytes_.size()); - DCHECK(type_ == TYPE_FILE); + DCHECK_EQ(TYPE_FILE, type_); + DCHECK(!file_stream_); // 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. + content_length_computed_ = true; + content_length_ = 0; + + // We need to open the file here to decide if we should report the file's + // size or zero. We cache the open file, so that we can still read it when + // it comes time to. + file_stream_ = NewFileStreamForReading(); + if (!file_stream_) + return 0; + int64 length = 0; if (!file_util::GetFileSize(file_path_, &length)) return 0; @@ -38,7 +50,40 @@ uint64 UploadData::Element::GetContentLength() const { return 0; // range is beyond eof // compensate for the offset and clip file_range_length_ to eof - return std::min(length - file_range_offset_, file_range_length_); + content_length_ = std::min(length - file_range_offset_, file_range_length_); + return content_length_; +} + +FileStream* UploadData::Element::NewFileStreamForReading() { + // In common usage GetContentLength() will call this first and store the + // result into |file_| and a subsequent call (from UploadDataStream) will + // get the cached open FileStream. + if (file_stream_) { + FileStream* file = file_stream_; + file_stream_ = NULL; + return file; + } + + scoped_ptr<FileStream> file(new FileStream()); + int64 rv = file->Open(file_path_, + base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ); + if (rv != OK) { + // If the file can't be opened, we'll just upload an empty file. + DLOG(WARNING) << "Failed to open \"" << file_path_.value() + << "\" for reading: " << rv; + return NULL; + } + if (file_range_offset_) { + rv = file->Seek(FROM_BEGIN, file_range_offset_); + if (rv < 0) { + DLOG(WARNING) << "Failed to seek \"" << file_path_.value() + << "\" to offset: " << file_range_offset_ << " (" << rv + << ")"; + return NULL; + } + } + + return file.release(); } } // namespace net diff --git a/net/base/upload_data.h b/net/base/upload_data.h index 869ab68..fe395f0 100644 --- a/net/base/upload_data.h +++ b/net/base/upload_data.h @@ -9,7 +9,9 @@ #include "base/basictypes.h" #include "base/file_path.h" +#include "base/logging.h" #include "base/ref_counted.h" +#include "net/base/file_stream.h" #include "base/time.h" #include "testing/gtest/include/gtest/gtest_prod.h" @@ -28,7 +30,14 @@ class UploadData : public base::RefCounted<UploadData> { public: Element() : type_(TYPE_BYTES), file_range_offset_(0), file_range_length_(kuint64max), - override_content_length_(false) { + override_content_length_(false), + content_length_computed_(false), + file_stream_(NULL) { + } + + ~Element() { + // In the common case |file__stream_| will be null. + delete file_stream_; } Type type() const { return type_; } @@ -65,7 +74,14 @@ class UploadData : public base::RefCounted<UploadData> { // 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; + // Once called, this function will always return the same value. + uint64 GetContentLength(); + + // Returns a FileStream opened for reading for this element, positioned at + // |file_range_offset_|. The caller gets ownership and is responsible + // for cleaning up the FileStream. Returns NULL if this element is not of + // type TYPE_FILE or if the file is not openable. + FileStream* NewFileStreamForReading(); private: // Allows tests to override the result of GetContentLength. @@ -81,7 +97,9 @@ class UploadData : public base::RefCounted<UploadData> { uint64 file_range_length_; base::Time expected_file_modification_time_; bool override_content_length_; + bool content_length_computed_; uint64 content_length_; + FileStream* file_stream_; FRIEND_TEST(UploadDataStreamTest, FileSmallerThanLength); FRIEND_TEST(HttpNetworkTransactionTest, UploadFileSmallerThanLength); @@ -108,10 +126,10 @@ class UploadData : public base::RefCounted<UploadData> { } // Returns the total size in bytes of the data to upload. - uint64 GetContentLength() const; + uint64 GetContentLength(); - const std::vector<Element>& elements() const { - return elements_; + std::vector<Element>* elements() { + return &elements_; } void set_elements(const std::vector<Element>& elements) { diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc index a3a67fa..140aa8a 100644 --- a/net/base/upload_data_stream.cc +++ b/net/base/upload_data_stream.cc @@ -11,8 +11,7 @@ namespace net { -UploadDataStream* UploadDataStream::Create(const UploadData* data, - int* error_code) { +UploadDataStream* UploadDataStream::Create(UploadData* data, int* error_code) { scoped_ptr<UploadDataStream> stream(new UploadDataStream(data)); int rv = stream->FillBuf(); if (error_code) @@ -23,11 +22,11 @@ UploadDataStream* UploadDataStream::Create(const UploadData* data, return stream.release(); } -UploadDataStream::UploadDataStream(const UploadData* data) +UploadDataStream::UploadDataStream(UploadData* data) : data_(data), buf_(new IOBuffer(kBufSize)), buf_len_(0), - next_element_(data->elements().begin()), + next_element_(data->elements()->begin()), next_element_offset_(0), next_element_remaining_(0), total_size_(data->GetContentLength()), @@ -52,13 +51,13 @@ void UploadDataStream::DidConsume(size_t num_bytes) { } int UploadDataStream::FillBuf() { - std::vector<UploadData::Element>::const_iterator end = - data_->elements().end(); + std::vector<UploadData::Element>::iterator end = + data_->elements()->end(); while (buf_len_ < kBufSize && next_element_ != end) { bool advance_to_next_element = false; - const UploadData::Element& element = *next_element_; + UploadData::Element& element = *next_element_; size_t size_remaining = kBufSize - buf_len_; if (element.type() == UploadData::TYPE_BYTES) { @@ -78,57 +77,51 @@ int UploadDataStream::FillBuf() { } else { DCHECK(element.type() == UploadData::TYPE_FILE); - // If the underlying file has been changed, treat it as error. - // Note that the expected modification time from WebKit is based on - // time_t precision. So we have to convert both to time_t to compare. - if (!element.expected_file_modification_time().is_null()) { - file_util::FileInfo info; - if (file_util::GetFileInfo(element.file_path(), &info) && - element.expected_file_modification_time().ToTimeT() != - info.last_modified.ToTimeT()) { - return ERR_UPLOAD_FILE_CHANGED; - } - } - - 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) { - 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() - << "\" to offset: " << offset; - } else { - next_element_remaining_ = element.file_range_length(); + if (!next_element_remaining_) { + // If the underlying file has been changed, treat it as error. + // Note that the expected modification time from WebKit is based on + // time_t precision. So we have to convert both to time_t to compare. + if (!element.expected_file_modification_time().is_null()) { + file_util::FileInfo info; + if (file_util::GetFileInfo(element.file_path(), &info) && + element.expected_file_modification_time().ToTimeT() != + info.last_modified.ToTimeT()) { + return ERR_UPLOAD_FILE_CHANGED; } } + next_element_remaining_ = element.GetContentLength(); + next_element_stream_.reset(element.NewFileStreamForReading()); } 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) { + int count = + static_cast<int>(std::min(next_element_remaining_, + static_cast<uint64>(size_remaining))); + if (count > 0) { + if (next_element_stream_.get()) + rv = next_element_stream_->Read(buf_->data() + buf_len_, count, NULL); + if (rv <= 0) { + // If there's less data to read than we initially observed, then + // pad with zero. Otherwise the server will hang waiting for the + // rest of the data. + memset(buf_->data() + buf_len_, 0, count); + rv = count; + } buf_len_ += rv; - next_element_remaining_ -= rv; - } else { + } + + if (static_cast<int>(next_element_remaining_) == rv) { advance_to_next_element = true; + } else { + next_element_remaining_ -= rv; } } if (advance_to_next_element) { ++next_element_; next_element_offset_ = 0; - next_element_stream_.Close(); + next_element_remaining_ = 0; + next_element_stream_.reset(); } } diff --git a/net/base/upload_data_stream.h b/net/base/upload_data_stream.h index b326dae..9fac07e 100644 --- a/net/base/upload_data_stream.h +++ b/net/base/upload_data_stream.h @@ -17,7 +17,7 @@ class UploadDataStream { // Returns a new instance of UploadDataStream if it can be created and // initialized successfully. If not, NULL will be returned and the error // code will be set if the output parameter error_code is not empty. - static UploadDataStream* Create(const UploadData* data, int* error_code); + static UploadDataStream* Create(UploadData* data, int* error_code); ~UploadDataStream(); @@ -44,14 +44,14 @@ class UploadDataStream { private: // Protects from public access since now we have a static creator function // which will do both creation and initialization and might return an error. - explicit UploadDataStream(const UploadData* data); + explicit UploadDataStream(UploadData* data); // Fills the buffer with any remaining data and sets eof_ if there was nothing // left to fill the buffer with. // Returns OK if the operation succeeds. Otherwise error code is returned. int 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 @@ -62,7 +62,7 @@ class UploadDataStream { size_t buf_len_; // Iterator to the upload element to be written to the send buffer next. - std::vector<UploadData::Element>::const_iterator next_element_; + std::vector<UploadData::Element>::iterator next_element_; // The byte offset into next_element_'s data buffer if the next element is // a TYPE_BYTES element. @@ -70,7 +70,7 @@ class UploadDataStream { // A stream to the currently open file, for next_element_ if the next element // is a TYPE_FILE element. - FileStream next_element_stream_; + scoped_ptr<FileStream> next_element_stream_; // The number of bytes remaining to be read from the currently open file // if the next element is of TYPE_FILE. diff --git a/net/base/upload_data_stream_unittest.cc b/net/base/upload_data_stream_unittest.cc index 701bad9..92c065e 100644 --- a/net/base/upload_data_stream_unittest.cc +++ b/net/base/upload_data_stream_unittest.cc @@ -78,7 +78,9 @@ TEST_F(UploadDataStreamTest, FileSmallerThanLength) { read_counter += stream->buf_len(); stream->DidConsume(stream->buf_len()); } - EXPECT_LT(read_counter, stream->size()); + // UpdateDataStream will pad out the file with 0 bytes so that the HTTP + // transaction doesn't hang. Therefore we expected the full size. + EXPECT_EQ(read_counter, stream->size()); file_util::Delete(temp_file_path, false); } diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc index b25dcfb..bef3237 100644 --- a/net/http/http_network_transaction_unittest.cc +++ b/net/http/http_network_transaction_unittest.cc @@ -8,6 +8,7 @@ #include "base/compiler_specific.h" #include "base/file_path.h" #include "base/file_util.h" +#include "base/scoped_ptr.h" #include "net/base/completion_callback.h" #include "net/base/mock_host_resolver.h" #include "net/base/request_priority.h" @@ -4128,6 +4129,154 @@ TEST_F(HttpNetworkTransactionTest, UploadFileSmallerThanLength) { file_util::Delete(temp_file_path, false); } +TEST_F(HttpNetworkTransactionTest, UploadUnreadableFile) { + // If we try to upload an unreadable file, the network stack should report + // the file size as zero and upload zero bytes for that file. + SessionDependencies session_deps; + scoped_ptr<HttpTransaction> trans( + new HttpNetworkTransaction(CreateSession(&session_deps))); + + FilePath temp_file; + ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file)); + std::string temp_file_content("Unreadable file."); + ASSERT_TRUE(file_util::WriteFile(temp_file, temp_file_content.c_str(), + temp_file_content.length())); + ASSERT_TRUE(file_util::MakeFileUnreadable(temp_file)); + + HttpRequestInfo request; + request.method = "POST"; + request.url = GURL("http://www.google.com/upload"); + request.upload_data = new UploadData; + request.load_flags = 0; + + std::vector<UploadData::Element> elements; + UploadData::Element element; + element.SetToFilePath(temp_file); + elements.push_back(element); + request.upload_data->set_elements(elements); + + MockRead data_reads[] = { + MockRead("HTTP/1.0 200 OK\r\n\r\n"), + MockRead(false, OK), + }; + MockWrite data_writes[] = { + MockWrite("POST /upload HTTP/1.1\r\n" + "Host: www.google.com\r\n" + "Connection: keep-alive\r\n" + "Content-Length: 0\r\n\r\n"), + MockWrite(false, OK), + }; + StaticSocketDataProvider data(data_reads, arraysize(data_reads), data_writes, + arraysize(data_writes)); + session_deps.socket_factory.AddSocketDataProvider(&data); + + TestCompletionCallback callback; + + int rv = trans->Start(&request, &callback, NULL); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback.WaitForResult(); + EXPECT_EQ(OK, rv); + + const HttpResponseInfo* response = trans->GetResponseInfo(); + EXPECT_TRUE(response != NULL); + EXPECT_TRUE(response->headers != NULL); + EXPECT_EQ("HTTP/1.0 200 OK", response->headers->GetStatusLine()); + + file_util::Delete(temp_file, false); +} + +TEST_F(HttpNetworkTransactionTest, UnreadableUploadFileAfterAuthRestart) { + SessionDependencies session_deps; + scoped_ptr<HttpTransaction> trans( + new HttpNetworkTransaction(CreateSession(&session_deps))); + + FilePath temp_file; + ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file)); + std::string temp_file_contents("Unreadable file."); + std::string unreadable_contents(temp_file_contents.length(), '\0'); + ASSERT_TRUE(file_util::WriteFile(temp_file, temp_file_contents.c_str(), + temp_file_contents.length())); + + HttpRequestInfo request; + request.method = "POST"; + request.url = GURL("http://www.google.com/upload"); + request.upload_data = new UploadData; + request.load_flags = 0; + + std::vector<UploadData::Element> elements; + UploadData::Element element; + element.SetToFilePath(temp_file); + elements.push_back(element); + request.upload_data->set_elements(elements); + + MockRead data_reads[] = { + MockRead("HTTP/1.1 401 Unauthorized\r\n"), + MockRead("WWW-Authenticate: Basic realm=\"MyRealm1\"\r\n"), + MockRead("Content-Length: 0\r\n\r\n"), // No response body. + + MockRead("HTTP/1.1 200 OK\r\n"), + MockRead("Content-Length: 0\r\n\r\n"), + MockRead(false, OK), + }; + MockWrite data_writes[] = { + MockWrite("POST /upload HTTP/1.1\r\n" + "Host: www.google.com\r\n" + "Connection: keep-alive\r\n" + "Content-Length: 16\r\n\r\n"), + MockWrite(false, temp_file_contents.c_str()), + + MockWrite("POST /upload HTTP/1.1\r\n" + "Host: www.google.com\r\n" + "Connection: keep-alive\r\n" + "Content-Length: 16\r\n" + "Authorization: Basic Zm9vOmJhcg==\r\n\r\n"), + MockWrite(false, unreadable_contents.c_str(), temp_file_contents.length()), + MockWrite(false, OK), + }; + StaticSocketDataProvider data(data_reads, arraysize(data_reads), data_writes, + arraysize(data_writes)); + session_deps.socket_factory.AddSocketDataProvider(&data); + + TestCompletionCallback callback1; + + int rv = trans->Start(&request, &callback1, NULL); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback1.WaitForResult(); + EXPECT_EQ(OK, rv); + + const HttpResponseInfo* response = trans->GetResponseInfo(); + EXPECT_TRUE(response != NULL); + EXPECT_TRUE(response->headers != NULL); + EXPECT_EQ("HTTP/1.1 401 Unauthorized", response->headers->GetStatusLine()); + + // The password prompt info should have been set in response->auth_challenge. + EXPECT_TRUE(response->auth_challenge.get() != NULL); + EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port); + EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm); + EXPECT_EQ(L"basic", response->auth_challenge->scheme); + + // Now make the file unreadable and try again. + ASSERT_TRUE(file_util::MakeFileUnreadable(temp_file)); + + TestCompletionCallback callback2; + + rv = trans->RestartWithAuth(L"foo", L"bar", &callback2); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback2.WaitForResult(); + EXPECT_EQ(OK, rv); + + response = trans->GetResponseInfo(); + EXPECT_TRUE(response != NULL); + EXPECT_TRUE(response->headers != NULL); + EXPECT_TRUE(response->auth_challenge.get() == NULL); + EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine()); + + file_util::Delete(temp_file, false); +} + // Tests that changes to Auth realms are treated like auth rejections. TEST_F(HttpNetworkTransactionTest, ChangeAuthRealms) { SessionDependencies session_deps; |