Skip to content

Commit

Permalink
PPAPI: Ensure FileIOResource has a valid FD for background thread ops.
Browse files Browse the repository at this point in the history 
BUG=83774

Review URL: https://codereview.chromium.org/25367002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@226535 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
dmichael@chromium.org committed Oct 2, 2013
1 parent e9eb7e2 commit d35d3f4
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 31 deletions.
57 changes: 34 additions & 23 deletions ppapi/proxy/file_io_resource.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,24 +43,26 @@ void DoClose(base::PlatformFile file) {
namespace ppapi {
namespace proxy {

FileIOResource::QueryOp::QueryOp(PP_FileHandle file_handle)
FileIOResource::QueryOp::QueryOp(scoped_refptr<FileHandleHolder> file_handle)
: file_handle_(file_handle) {
DCHECK(file_handle_);
}

FileIOResource::QueryOp::~QueryOp() {
}

int32_t FileIOResource::QueryOp::DoWork() {
return base::GetPlatformFileInfo(file_handle_, &file_info_) ?
return base::GetPlatformFileInfo(file_handle_->raw_handle(), &file_info_) ?
PP_OK : PP_ERROR_FAILED;
}

FileIOResource::ReadOp::ReadOp(PP_FileHandle file_handle,
FileIOResource::ReadOp::ReadOp(scoped_refptr<FileHandleHolder> file_handle,
int64_t offset,
int32_t bytes_to_read)
: file_handle_(file_handle),
offset_(offset),
bytes_to_read_(bytes_to_read) {
DCHECK(file_handle_);
}

FileIOResource::ReadOp::~ReadOp() {
Expand All @@ -70,18 +72,16 @@ int32_t FileIOResource::ReadOp::DoWork() {
DCHECK(!buffer_.get());
buffer_.reset(new char[bytes_to_read_]);
return base::ReadPlatformFile(
file_handle_, offset_, buffer_.get(), bytes_to_read_);
file_handle_->raw_handle(), offset_, buffer_.get(), bytes_to_read_);
}

FileIOResource::FileIOResource(Connection connection, PP_Instance instance)
: PluginResource(connection, instance),
file_handle_(base::kInvalidPlatformFileValue),
file_system_type_(PP_FILESYSTEMTYPE_INVALID) {
SendCreate(RENDERER, PpapiHostMsg_FileIO_Create());
}

FileIOResource::~FileIOResource() {
CloseFileHandle();
}

PPB_FileIO_API* FileIOResource::AsPPB_FileIO_API() {
Expand Down Expand Up @@ -134,7 +134,7 @@ int32_t FileIOResource::Query(PP_FileInfo* info,
return rv;
if (!info)
return PP_ERROR_BADARGUMENT;
if (file_handle_ == base::kInvalidPlatformFileValue)
if (!FileHandleHolder::IsValid(file_handle_))
return PP_ERROR_FAILED;

state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
Expand Down Expand Up @@ -262,7 +262,9 @@ int32_t FileIOResource::Flush(scoped_refptr<TrackedCallback> callback) {
}

void FileIOResource::Close() {
CloseFileHandle();
if (file_handle_) {
file_handle_ = NULL;
}
Post(RENDERER, PpapiHostMsg_FileIO_Close());
}

Expand Down Expand Up @@ -291,13 +293,32 @@ int32_t FileIOResource::RequestOSFileHandle(
return PP_OK_COMPLETIONPENDING;
}

FileIOResource::FileHandleHolder::FileHandleHolder(PP_FileHandle file_handle)
: raw_handle_(file_handle) {
}

// static
bool FileIOResource::FileHandleHolder::IsValid(
const scoped_refptr<FileIOResource::FileHandleHolder>& handle) {
return handle && (handle->raw_handle() != base::kInvalidPlatformFileValue);
}

FileIOResource::FileHandleHolder::~FileHandleHolder() {
if (raw_handle_ != base::kInvalidPlatformFileValue) {
base::TaskRunner* file_task_runner =
PpapiGlobals::Get()->GetFileTaskRunner();
file_task_runner->PostTask(FROM_HERE,
base::Bind(&DoClose, raw_handle_));
}
}

int32_t FileIOResource::ReadValidated(int64_t offset,
int32_t bytes_to_read,
const PP_ArrayOutput& array_output,
scoped_refptr<TrackedCallback> callback) {
if (bytes_to_read < 0)
return PP_ERROR_FAILED;
if (file_handle_ == base::kInvalidPlatformFileValue)
if (!FileHandleHolder::IsValid(file_handle_))
return PP_ERROR_FAILED;

state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_READ);
Expand Down Expand Up @@ -327,18 +348,6 @@ int32_t FileIOResource::ReadValidated(int64_t offset,
return PP_OK_COMPLETIONPENDING;
}

void FileIOResource::CloseFileHandle() {
if (file_handle_ != base::kInvalidPlatformFileValue) {
// Close our local fd on the file thread.
base::TaskRunner* file_task_runner =
PpapiGlobals::Get()->GetFileTaskRunner();
file_task_runner->PostTask(FROM_HERE,
base::Bind(&DoClose, file_handle_));

file_handle_ = base::kInvalidPlatformFileValue;
}
}

int32_t FileIOResource::OnQueryComplete(scoped_refptr<QueryOp> query_op,
PP_FileInfo* info,
int32_t result) {
Expand Down Expand Up @@ -401,8 +410,10 @@ void FileIOResource::OnPluginMsgOpenFileComplete(

int32_t result = params.result();
IPC::PlatformFileForTransit transit_file;
if ((result == PP_OK) && params.TakeFileHandleAtIndex(0, &transit_file))
file_handle_ = IPC::PlatformFileForTransitToPlatformFile(transit_file);
if ((result == PP_OK) && params.TakeFileHandleAtIndex(0, &transit_file)) {
file_handle_ = new FileHandleHolder(
IPC::PlatformFileForTransitToPlatformFile(transit_file));
}
// End this operation now, so the user's callback can execute another FileIO
// operation, assuming there are no other pending operations.
state_manager_.SetOperationFinished();
Expand Down
49 changes: 41 additions & 8 deletions ppapi/proxy/file_io_resource.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <string>

#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "ppapi/c/private/pp_file_handle.h"
#include "ppapi/proxy/connection.h"
Expand Down Expand Up @@ -63,10 +64,43 @@ class PPAPI_PROXY_EXPORT FileIOResource
scoped_refptr<TrackedCallback> callback) OVERRIDE;

private:
// FileHandleHolder is used to guarantee that file operations will have a
// valid FD to operate on, even if they're in a different thread.
// If instead we just passed the raw FD, the FD could be closed before the
// file operation has a chance to run. It could interact with an invalid FD,
// or worse, the FD value could be reused if another file is opened quickly
// (POSIX is required to provide the lowest available value when opening a
// file). This could result in strange problems such as writing data to the
// wrong file.
//
// Operations that run on a background thread should hold one of these to
// ensure they have a valid file descriptor. The file handle is only closed
// when the last reference to the FileHandleHolder is removed, so we are
// guaranteed to operate on the correct file descriptor. It *is* still
// possible that the FileIOResource will be destroyed and "Abort" callbacks
// just before the operation does its task (e.g., Reading). In that case, we
// might for example Read from a file even though the FileIO has been
// destroyed and the plugin's callback got a PP_ERROR_ABORTED result. In the
// case of a write, we could write some data to the file despite the plugin
// receiving a PP_ERROR_ABORTED instead of a successful result.
class FileHandleHolder : public base::RefCountedThreadSafe<FileHandleHolder> {
public:
explicit FileHandleHolder(PP_FileHandle file_handle_);
PP_FileHandle raw_handle() {
return raw_handle_;
}
static bool IsValid(
const scoped_refptr<FileIOResource::FileHandleHolder>& handle);
private:
friend class base::RefCountedThreadSafe<FileHandleHolder>;
~FileHandleHolder();
PP_FileHandle raw_handle_;
};

// Class to perform file query operations across multiple threads.
class QueryOp : public base::RefCountedThreadSafe<QueryOp> {
public:
explicit QueryOp(PP_FileHandle file_handle);
explicit QueryOp(scoped_refptr<FileHandleHolder> file_handle);

// Queries the file. Called on the file thread (non-blocking) or the plugin
// thread (blocking). This should not be called when we hold the proxy lock.
Expand All @@ -78,14 +112,16 @@ class PPAPI_PROXY_EXPORT FileIOResource
friend class base::RefCountedThreadSafe<QueryOp>;
~QueryOp();

PP_FileHandle file_handle_;
scoped_refptr<FileHandleHolder> file_handle_;
base::PlatformFileInfo file_info_;
};

// Class to perform file read operations across multiple threads.
class ReadOp : public base::RefCountedThreadSafe<ReadOp> {
public:
ReadOp(PP_FileHandle file_handle, int64_t offset, int32_t bytes_to_read);
ReadOp(scoped_refptr<FileHandleHolder> file_handle,
int64_t offset,
int32_t bytes_to_read);

// Reads the file. Called on the file thread (non-blocking) or the plugin
// thread (blocking). This should not be called when we hold the proxy lock.
Expand All @@ -97,7 +133,7 @@ class PPAPI_PROXY_EXPORT FileIOResource
friend class base::RefCountedThreadSafe<ReadOp>;
~ReadOp();

PP_FileHandle file_handle_;
scoped_refptr<FileHandleHolder> file_handle_;
int64_t offset_;
int32_t bytes_to_read_;
scoped_ptr<char[]> buffer_;
Expand All @@ -108,9 +144,6 @@ class PPAPI_PROXY_EXPORT FileIOResource
const PP_ArrayOutput& array_output,
scoped_refptr<TrackedCallback> callback);

void CloseFileHandle();


// Completion tasks for file operations that are done in the plugin.
int32_t OnQueryComplete(scoped_refptr<QueryOp> query_op,
PP_FileInfo* info,
Expand All @@ -129,7 +162,7 @@ class PPAPI_PROXY_EXPORT FileIOResource
PP_FileHandle* output_handle,
const ResourceMessageReplyParams& params);

PP_FileHandle file_handle_;
scoped_refptr<FileHandleHolder> file_handle_;
PP_FileSystemType file_system_type_;
FileIOStateManager state_manager_;

Expand Down

0 comments on commit d35d3f4

Please sign in to comment.