PPAPI: Implement PPB_FileMapping on POSIX

ppapi/proxy/file_mapping_resource.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[teravest, 2014/01/23 21:40:58]

boring nit: 2014 :(

[dmichael (off chromium), 2014/01/24 20:02:50]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ppapi/proxy/file_mapping_resource.h"
+
+#if defined(OS_POSIX)

[teravest, 2014/01/23 21:40:58]

It would be cool if there was an abstraction you could use for DoMap and DoUnmap
that put logic for different platforms in different files (like we have for ipc
channels or shared memory).

I understand if you don't want to bother with that, though.

[dmichael (off chromium), 2014/01/24 20:02:50]

Yeah...  I had already considered it, and was thinking maybe for next CL. But I
went ahead and did it this time. I think it's cleaner split up. Also did it for
GetMapPageSize, just for consistency.

+#include <stdio.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#endif
+
+#include <limits>
+
+#include "base/bind.h"
+#include "base/numerics/safe_conversions.h"
+#include "base/task_runner_util.h"
+#include "ppapi/shared_impl/tracked_callback.h"
+#include "ppapi/shared_impl/var.h"
+#include "ppapi/thunk/enter.h"
+#include "ppapi/thunk/ppb_file_io_api.h"
+
+namespace ppapi {
+namespace proxy {
+
+namespace {
+
+int32_t ErrnoToPPError(int error_code) {
+  switch (error_code) {
+    case EACCES:
+      return PP_ERROR_NOACCESS;
+    case EAGAIN:
+      return PP_ERROR_NOMEMORY;
+    case EINVAL:
+      return PP_ERROR_BADARGUMENT;
+    case ENFILE:
+    case ENOMEM:
+      return PP_ERROR_NOMEMORY;
+    default:
+      return PP_ERROR_FAILED;
+  }
+}
+
+}
+
+FileMappingResource::FileMappingResource(Connection connection,
+                                         PP_Instance instance)
+    : PluginResource(connection, instance) {
+}
+
+FileMappingResource::~FileMappingResource() {
+}
+
+thunk::PPB_FileMapping_API* FileMappingResource::AsPPB_FileMapping_API() {
+  return this;
+}
+
+int32_t FileMappingResource::Map(PP_Instance /* instance */,
+                                 PP_Resource file_io,
+                                 int64_t length,
+                                 uint32_t map_protection,
+                                 uint32_t map_flags,
+                                 int64_t offset,
+                                 void** address,
+                                 scoped_refptr<TrackedCallback> callback) {
+  thunk::EnterResourceNoLock<thunk::PPB_FileIO_API> enter(file_io, true);
+  if (enter.failed())
+    return PP_ERROR_BADARGUMENT;
+  FileIOResource* file_io_resource =
+      static_cast<FileIOResource*>(enter.object());
+  scoped_refptr<FileIOResource::FileHandleHolder> file_handle =
+      file_io_resource->file_handle();
+  if (!FileIOResource::FileHandleHolder::IsValid(file_handle))
+    return PP_ERROR_BADARGUMENT;

[bbudge, 2014/01/23 23:01:03]

Kind of a gray area, but this could also be PP_ERROR_FAILED.

[dmichael (off chromium), 2014/01/24 20:02:50]

I think of PP_ERROR_FAILED as a more generic, "we screwed up but don't know what
happened". I think this can happen if you pass a FileIO that's either not been
opened or has been closed, so that's why I was thinking BADARGUMENT. WDYT?

[bbudge, 2014/01/24 21:41:21]

In retrospect, we should probably have defined a PP_ERROR_FILENOTOPEN.

Returning PP_ERROR_FAILED would be consistent with FileIO - we return it for any
operation when the file isn't open. It might be better - I don't know under what
conditions map/unmap might fail. If that is unusual then this might be better.

PP_ERROR_BADARGUMENT is also a reasonable choice here. I leave it to you.

[dmichael (off chromium), 2014/01/24 22:05:13]

Okay, I'm convinced. Done.

+  if (length < 0 || offset < 0)
+    return PP_ERROR_BADARGUMENT;
+  if (!base::IsValueInRangeForNumericType<size_t>(length))
+    return PP_ERROR_NOMEMORY;

[teravest, 2014/01/23 21:40:58]

I'd argue that this should be BADARGUMENT. But either way is ok I guess.

[dmichael (off chromium), 2014/01/24 20:02:50]

This is for platform consistency. On 64-bit, values over 2^32 will still fit in
size_t, so we would pass it along to mmap, which will usually result in
PP_ERROR_NOMEMORY. So this check comes in to play on 32-bit, and I wanted it to
have the same failure code there. (This came up in my testing, where I pass
numeric_limits<int64_t>::max()).

+  if (!base::IsValueInRangeForNumericType<off_t>(offset))
+    return PP_ERROR_BADARGUMENT;
+
+  // Ensure no bits that we don't recognize are set.
+  if (map_protection &
+      ~(PP_FILEMAPPROTECTION_READ | PP_FILEMAPPROTECTION_WRITE)) {
+    return PP_ERROR_BADARGUMENT;
+  }
+  if (map_flags &
+      ~(PP_FILEMAPFLAG_SHARED | PP_FILEMAPFLAG_PRIVATE |
+        PP_FILEMAPFLAG_FIXED)) {
+    return PP_ERROR_BADARGUMENT;
+  }
+  // Ensure at least one of SHARED and PRIVATE is set.
+  if (!(map_flags & (PP_FILEMAPFLAG_SHARED | PP_FILEMAPFLAG_PRIVATE)))
+    return PP_ERROR_BADARGUMENT;
+  // Ensure at most one of SHARED and PRIVATE is set.
+  if ((map_flags & PP_FILEMAPFLAG_SHARED) &&
+      (map_flags & PP_FILEMAPFLAG_PRIVATE)) {
+    return PP_ERROR_BADARGUMENT;
+  }
+  if (!address)
+    return PP_ERROR_BADARGUMENT;
+
+  base::Callback<MapResult()> map_op(

[bbudge, 2014/01/24 21:41:21]

Maybe a better name is map_task or map_cb?
Same for unmap_op below.

+      base::Bind(&FileMappingResource::DoMap, file_handle, *address, length,
+                 map_protection, map_flags, offset));
+  if (callback->is_blocking()) {
+    // The plugin could release its reference to this instance when we release
+    // the proxy lock below.
+    scoped_refptr<FileMappingResource> protect(this);
+    MapResult map_result;
+    {
+      // Release the proxy lock while making a potentially slow file call.
+      ProxyAutoUnlock unlock;
+      map_result = map_op.Run();
+    }
+    OnMapCompleted(address, callback, map_result);
+    return map_result.result;
+  } else {
+    base::PostTaskAndReplyWithResult(
+      PpapiGlobals::Get()->GetFileTaskRunner(),
+      FROM_HERE,
+      map_op,
+      RunWhileLocked(Bind(&FileMappingResource::OnMapCompleted,
+                          this,
+                          base::Unretained(address),
+                          callback)));
+    return PP_OK_COMPLETIONPENDING;
+  }
+}
+
+int32_t FileMappingResource::Unmap(PP_Instance /* instance */,
+                                   const void* address,
+                                   int64_t length,
+                                   scoped_refptr<TrackedCallback> callback) {
+  if (!address)
+    return PP_ERROR_BADARGUMENT;
+  if (!base::IsValueInRangeForNumericType<size_t>(length))
+    return PP_ERROR_BADARGUMENT;
+
+  base::Callback<int32_t()> unmap_op(
+      base::Bind(&FileMappingResource::DoUnmap, address, length));
+  if (callback->is_blocking()) {
+    // Release the proxy lock while making a potentially slow file call.
+    ProxyAutoUnlock unlock;
+    return unmap_op.Run();
+  } else {
+    base::PostTaskAndReplyWithResult(
+      PpapiGlobals::Get()->GetFileTaskRunner(),
+      FROM_HERE,
+      unmap_op,
+      RunWhileLocked(Bind(&TrackedCallback::Run, callback)));
+    return PP_OK_COMPLETIONPENDING;
+  }
+}
+
+int64_t FileMappingResource::GetMapPageSize(PP_Instance /* instance */) {
+  // TODO(dmichael): It might be better to port sys_info_posix.cc to NaCl and
+  // call Sysinfo::VMAllocationGranularity.
+#if defined(OS_POSIX)
+  return getpagesize();
+#else  // defined(OS_POSIX)
+  NOTREACHED();

[bbudge, 2014/01/23 23:01:03]

NOTIMPLEMENTED()?

[dmichael (off chromium), 2014/01/24 20:02:50]

Oops, the other Windows functions just return PP_ERROR_NOTSUPPORTED without the
check...  so I just removed NOTREACHED here for consistency.

But if you think I should have the NOTIMPLEMENTED in all of these, I'm happy to
do that. I suspect it won't make a lot of difference either way.

[bbudge, 2014/01/24 21:41:21]

No big deal.

+  return 0;
+#endif
+}
+
+// static
+FileMappingResource::MapResult FileMappingResource::DoMap(
+    scoped_refptr<FileIOResource::FileHandleHolder> handle,
+    void* address_hint,
+    int64_t length,
+    uint32_t map_protection,
+    uint32_t map_flags,
+    int64_t offset) {
+#if defined(OS_POSIX)
+  int prot_for_mmap = 0;
+  if (map_protection & PP_FILEMAPPROTECTION_READ)
+    prot_for_mmap |= PROT_READ;
+  if (map_protection & PP_FILEMAPPROTECTION_WRITE)
+    prot_for_mmap |= PROT_WRITE;
+  if (prot_for_mmap == 0)
+    prot_for_mmap = PROT_NONE;
+
+  int flags_for_mmap = 0;
+  if (map_flags & PP_FILEMAPFLAG_SHARED)
+    flags_for_mmap |= MAP_SHARED;
+  if (map_flags & PP_FILEMAPFLAG_PRIVATE)
+    flags_for_mmap |= MAP_PRIVATE;
+  if (map_flags & PP_FILEMAPFLAG_FIXED)
+    flags_for_mmap |= MAP_FIXED;
+
+  MapResult map_result;
+  map_result.address =
+      mmap(address_hint,
+           static_cast<size_t>(length),
+           prot_for_mmap,
+           flags_for_mmap,
+           handle->raw_handle(),
+           static_cast<off_t>(offset));
+  if (map_result.address != MAP_FAILED)
+    map_result.result = PP_OK;
+  else
+    map_result.result = ErrnoToPPError(errno);

[bbudge, 2014/01/23 23:01:03]

nit: Seems like you could access this inside ErrnoToPPError rather than at call
sites.

[dmichael (off chromium), 2014/01/24 20:02:50]

Yeah...  I thought about that, but I already hate that it's a global variable,
and I thought this was a little cleaner/more flexible than relying on the global
state. E.g., you could stash an errno value to call it with at a later time
(though admittedly I don't use that flexibility).

[bbudge, 2014/01/24 21:41:21]

OK

+  return map_result;
+#else  // defined(OS_POSIX)
+  // TODO(dmichael): Implement for Windows (crbug.com/83774).
+  MapResult map_result;
+  map_result.result = PP_ERROR_NOTSUPPORTED;
+  return map_result;
+#endif
+}
+
+void FileMappingResource::OnMapCompleted(
+    void** mapped_address_out_param,
+    scoped_refptr<TrackedCallback> callback,
+    const MapResult& map_result) {
+  // TODO(dmichael): If the callback is aborted, should we unmap?

[teravest, 2014/01/23 21:40:58]

I think we should unmap when the callback is aborted.

When the callback isn't invoked, the user won't have an address to use for
calling Unmap, so we should unmap in that case to prevent leaks.

[dmichael (off chromium), 2014/01/24 20:02:50]

Good point... they do get a PP_ERROR_ABORTED callback, but I wasn't thinking
about the fact that they don't get an address.

Done.

+  if (callback->aborted())
+    return;
+  if (map_result.result == 0)

[bbudge, 2014/01/23 23:01:03]

Since this is logically a PP_Error, PP_OK?

[dmichael (off chromium), 2014/01/24 20:02:50]

Done.

+    *mapped_address_out_param = map_result.address;
+  if (!callback->is_blocking())
+    callback->Run(map_result.result);
+}
+
+// static
+int32_t FileMappingResource::DoUnmap(const void* address, int64_t length) {
+#if defined(OS_POSIX)
+  int result = munmap(const_cast<void*>(address), static_cast<size_t>(length));
+  if (result)
+    return ErrnoToPPError(errno);
+  return PP_OK;
+#else
+  // TODO(dmichael): Implement for Windows (crbug.com/83774).
+  return PP_ERROR_NOTSUPPORTED;
+#endif
+}
+
+}  // namespace proxy
+}  // namespace ppapi