Skip to content

Commit

Permalink
Make PlatformFileForTransit its own class on Windows.
Browse files Browse the repository at this point in the history
This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

The leak was introduced by "Make PlatformFileForTransit a class on Windows."
[commit: 19e5f69]. Prior to that CL:
  On Windows, NaCl would duplicate the handle into the destination process
  [closing the original handle in the process], and then pass the result as a
  raw int.

  On POSIX, NaCl would duplicate the handle using Chrome IPC, using
  base::FileDescriptor.auto_close = true, which would cause the IPC subsystem
  to close the handle.

After that CL:
  On Windows, NaCl would let Chrome IPC transport the handle, but failed to
  call PlatformFileForTransit::SetOwnershipPassesToIPC(true) [the equivalent of
  base::FileDescriptor.auto_close]. This caused a leak of the handle in the
  browser process.

In this CL:
  PlatformFileForTransit is now its own class, with no auto_close parameter
  because the IPC subsystem always closes the handle in the source process.

BUG=713763

Review-Url: https://codereview.chromium.org/2846293002
Cr-Commit-Position: refs/heads/master@{#468213}
  • Loading branch information
erikchen authored and Commit bot committed Apr 29, 2017
1 parent 7988a5c commit d804e10
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 14 deletions.
3 changes: 1 addition & 2 deletions components/nacl/renderer/ppb_nacl_private_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,7 @@ void PPBNaClPrivate::LaunchSelLdr(
if (nexe_file_info->handle != PP_kInvalidFileHandle)
nexe_for_transit = base::FileDescriptor(nexe_file_info->handle, true);
#elif defined(OS_WIN)
nexe_for_transit = IPC::PlatformFileForTransit(nexe_file_info->handle,
base::GetCurrentProcId());
nexe_for_transit = IPC::PlatformFileForTransit(nexe_file_info->handle);
#else
# error Unsupported target platform.
#endif
Expand Down
43 changes: 43 additions & 0 deletions ipc/ipc_message_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#if defined(OS_WIN)
#include <tchar.h>
#include "ipc/handle_win.h"
#include "ipc/ipc_platform_file.h"
#endif

namespace IPC {
Expand Down Expand Up @@ -842,6 +843,48 @@ void ParamTraits<base::SharedMemoryHandle>::Log(const param_type& p,
}
#endif // defined(OS_MACOSX) && !defined(OS_IOS)

#if defined(OS_WIN)
void ParamTraits<PlatformFileForTransit>::GetSize(base::PickleSizer* s,
const param_type& p) {
GetParamSize(s, p.IsValid());
if (p.IsValid())
GetParamSize(s, p.GetHandle());
}

void ParamTraits<PlatformFileForTransit>::Write(base::Pickle* m,
const param_type& p) {
m->WriteBool(p.IsValid());
if (p.IsValid()) {
HandleWin handle_win(p.GetHandle(), HandleWin::DUPLICATE);
ParamTraits<HandleWin>::Write(m, handle_win);
::CloseHandle(p.GetHandle());
}
}

bool ParamTraits<PlatformFileForTransit>::Read(const base::Pickle* m,
base::PickleIterator* iter,
param_type* r) {
bool is_valid;
if (!iter->ReadBool(&is_valid))
return false;
if (!is_valid) {
*r = PlatformFileForTransit();
return true;
}

HandleWin handle_win;
if (!ParamTraits<HandleWin>::Read(m, iter, &handle_win))
return false;
*r = PlatformFileForTransit(handle_win.get_handle());
return true;
}

void ParamTraits<PlatformFileForTransit>::Log(const param_type& p,
std::string* l) {
LogParam(p.GetHandle(), l);
}
#endif // defined(OS_WIN)

void ParamTraits<base::FilePath>::GetSize(base::PickleSizer* sizer,
const param_type& p) {
p.GetSizeForPickle(sizer);
Expand Down
17 changes: 17 additions & 0 deletions ipc/ipc_message_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ namespace IPC {

struct ChannelHandle;

#if defined(OS_WIN)
class PlatformFileForTransit;
#endif

// -----------------------------------------------------------------------------
// How we send IPC message logs across channels.
struct IPC_EXPORT LogData {
Expand Down Expand Up @@ -593,6 +597,19 @@ struct IPC_EXPORT ParamTraits<base::SharedMemoryHandle> {
static void Log(const param_type& p, std::string* l);
};

#if defined(OS_WIN)
template <>
struct IPC_EXPORT ParamTraits<PlatformFileForTransit> {
typedef PlatformFileForTransit param_type;
static void GetSize(base::PickleSizer* sizer, const param_type& p);
static void Write(base::Pickle* m, const param_type& p);
static bool Read(const base::Pickle* m,
base::PickleIterator* iter,
param_type* r);
static void Log(const param_type& p, std::string* l);
};
#endif // defined(OS_WIN)

template <>
struct IPC_EXPORT ParamTraits<base::FilePath> {
typedef base::FilePath param_type;
Expand Down
31 changes: 27 additions & 4 deletions ipc/ipc_platform_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,32 @@

namespace IPC {

#if defined(OS_WIN)
PlatformFileForTransit::PlatformFileForTransit() : handle_(nullptr) {}

PlatformFileForTransit::PlatformFileForTransit(HANDLE handle)
: handle_(handle) {}

bool PlatformFileForTransit::operator==(
const PlatformFileForTransit& platform_file) const {
return handle_ == platform_file.handle_;
}

bool PlatformFileForTransit::operator!=(
const PlatformFileForTransit& platform_file) const {
return !(*this == platform_file);
}

HANDLE PlatformFileForTransit::GetHandle() const {
return handle_;
}

bool PlatformFileForTransit::IsValid() const {
return handle_ != nullptr;
}

#endif // defined(OS_WIN)

PlatformFileForTransit GetPlatformFileForTransit(base::PlatformFile handle,
bool close_source_handle) {
#if defined(OS_WIN)
Expand All @@ -24,10 +50,7 @@ PlatformFileForTransit GetPlatformFileForTransit(base::PlatformFile handle,
return IPC::InvalidPlatformFileForTransit();
}

IPC::PlatformFileForTransit out_handle = IPC::PlatformFileForTransit(
raw_handle, base::GetCurrentProcId());
out_handle.SetOwnershipPassesToIPC(true);
return out_handle;
return IPC::PlatformFileForTransit(raw_handle);
#elif defined(OS_POSIX)
// If asked to close the source, we can simply re-use the source fd instead of
// dup()ing and close()ing.
Expand Down
31 changes: 23 additions & 8 deletions ipc/ipc_platform_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,32 @@
#include "base/file_descriptor_posix.h"
#endif

#if defined(OS_WIN)
#include "base/memory/shared_memory_handle.h"
#endif

namespace IPC {

#if defined(OS_WIN)
// The semantics for IPC transfer of a SharedMemoryHandle are exactly the same
// as for a PlatformFileForTransit. The object wraps a HANDLE, and has some
// metadata that indicates the process to which the HANDLE belongs.
using PlatformFileForTransit = base::SharedMemoryHandle;
class IPC_EXPORT PlatformFileForTransit {
public:
// Creates an invalid platform file.
PlatformFileForTransit();

// Creates a platform file that takes unofficial ownership of |handle|. Note
// that ownership is not handled by a Scoped* class due to usage patterns of
// this class and its POSIX counterpart [base::FileDescriptor]. When this
// class is used as an input to an IPC message, the IPC subsystem will close
// |handle|. When this class is used as the output from an IPC message, the
// receiver is expected to take ownership of |handle|.
explicit PlatformFileForTransit(HANDLE handle);

// Comparison operators.
bool operator==(const PlatformFileForTransit& platform_file) const;
bool operator!=(const PlatformFileForTransit& platform_file) const;

HANDLE GetHandle() const;
bool IsValid() const;

private:
HANDLE handle_;
};
#elif defined(OS_POSIX)
typedef base::FileDescriptor PlatformFileForTransit;
#endif
Expand Down

0 comments on commit d804e10

Please sign in to comment.