diff options
| author | satorux@chromium.org <satorux@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-02-24 20:48:24 +0000 |
|---|---|---|
| committer | satorux@chromium.org <satorux@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-02-24 20:48:24 +0000 |
| commit | 285673c88b5aab21ffe217e27ad182950209c928 (patch) | |
| tree | a190f18fb3dfecf4080fb81b38e04f863fcb3a71 /net | |
| parent | 7070b7f7a99f5ad9ebcf256d43883fb175f79578 (diff) | |
| download | chromium_src-285673c88b5aab21ffe217e27ad182950209c928.zip chromium_src-285673c88b5aab21ffe217e27ad182950209c928.tar.gz chromium_src-285673c88b5aab21ffe217e27ad182950209c928.tar.bz2 | |
net: Fix a bug in FileStream::CloseSync() on POSIX.
Before this patch, CloseSync() was forcibly closing the file without
checking if there is an in-flight async operation. This was unsafe and
resulted in writing data to a closed file descriptor, which then
resulted in flaky failures from nacl integration tests on Mac.
BUG=115561
TEST=add a test and run net_unittests
Review URL: https://chromiumcodereview.appspot.com/9447040
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@123540 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net')
| -rw-r--r-- | net/base/file_stream_posix.cc | 8 | ||||
| -rw-r--r-- | net/base/file_stream_unittest.cc | 21 |
2 files changed, 29 insertions, 0 deletions
diff --git a/net/base/file_stream_posix.cc b/net/base/file_stream_posix.cc index ae8780b..9c00bf3 100644 --- a/net/base/file_stream_posix.cc +++ b/net/base/file_stream_posix.cc @@ -251,6 +251,12 @@ void FileStreamPosix::CloseSync() { // // DCHECK(!(open_flags_ & base::PLATFORM_FILE_ASYNC)); weak_ptr_factory_.InvalidateWeakPtrs(); + // Block until the last read/write operation is complete, if needed. + // This is needed to close the file safely. + if (on_io_complete_.get()) { + on_io_complete_->Wait(); + on_io_complete_.reset(); // So the destructor won't be stuck. + } CloseFile(file_, bound_net_log_); file_ = base::kInvalidPlatformFileValue; @@ -423,6 +429,7 @@ int FileStreamPosix::Write( return ERR_UNEXPECTED; DCHECK(open_flags_ & base::PLATFORM_FILE_ASYNC); + DCHECK(open_flags_ & base::PLATFORM_FILE_WRITE); // write(..., 0) will return 0, which indicates end-of-file. DCHECK_GT(buf_len, 0); // Make sure we don't have a request in flight. @@ -452,6 +459,7 @@ int FileStreamPosix::WriteSync( return ERR_UNEXPECTED; DCHECK(!(open_flags_ & base::PLATFORM_FILE_ASYNC)); + DCHECK(open_flags_ & base::PLATFORM_FILE_WRITE); // write(..., 0) will return 0, which indicates end-of-file. DCHECK_GT(buf_len, 0); diff --git a/net/base/file_stream_unittest.cc b/net/base/file_stream_unittest.cc index 093659d..8f972ee 100644 --- a/net/base/file_stream_unittest.cc +++ b/net/base/file_stream_unittest.cc @@ -1095,6 +1095,27 @@ TEST_F(FileStreamTest, AsyncCloseTwice) { EXPECT_FALSE(stream.IsOpen()); } +// TODO(satorux): This should be gone once all once all async clients are +// migrated to use Close(). crbug.com/114783 +TEST_F(FileStreamTest, AsyncWriteAndCloseSync) { + FileStream stream(NULL); + int flags = base::PLATFORM_FILE_OPEN | + base::PLATFORM_FILE_WRITE | + base::PLATFORM_FILE_ASYNC; + TestCompletionCallback callback; + int rv = stream.Open(temp_file_path(), flags, callback.callback()); + EXPECT_EQ(ERR_IO_PENDING, rv); + EXPECT_EQ(OK, callback.WaitForResult()); + EXPECT_TRUE(stream.IsOpen()); + + // Write some data asynchronously. + scoped_refptr<IOBufferWithSize> buf = CreateTestDataBuffer(); + stream.Write(buf, buf->size(), callback.callback()); + + // Close the stream without waiting for the completion. + stream.CloseSync(); +} + } // namespace } // namespace net |