Pepper: DescWrapper cleanup in PnaclResources.

This is part of an effort to get rid of use of DescWrapper/NaClDesc for nexe
module loading. This is anticipation of moving nexe loading away from SRPC to
Chrome IPC.

Note that this tightens up some permissions for converting file descriptors;
I'm not sure why some were originally opened with _O_RDWR.

BUG=333950
R=bbudge@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271918 0039d316-1c4b-4281-b951-d872f2087c98

ppapi/native_client/src/trusted/plugin/file_downloader.cc

@@ -187,7 +187,7 @@ void FileDownloader::OpenFast(const nacl::string& url,
   mode_ = DOWNLOAD_NONE;
   if (file_handle != PP_kInvalidFileHandle) {
     NaClFileInfo tmp_info = NoFileInfo();
-    tmp_info.desc = ConvertFileDescriptor(file_handle);
+    tmp_info.desc = ConvertFileDescriptor(file_handle, true);
     tmp_info.file_token.lo = file_token_lo;
     tmp_info.file_token.hi = file_token_hi;
     file_info_.TakeOwnership(&tmp_info);
@@ -407,7 +407,7 @@ void FileDownloader::GotFileHandleNotify(int32_t pp_error,
       pp_error));
   if (pp_error == PP_OK) {
     NaClFileInfo tmp_info = NoFileInfo();
-    tmp_info.desc = ConvertFileDescriptor(handle);
+    tmp_info.desc = ConvertFileDescriptor(handle, false);
     file_info_.TakeOwnership(&tmp_info);
   }
 

ppapi/native_client/src/trusted/plugin/plugin.cc

@@ -49,27 +49,6 @@ const int64_t kTimeSmallMin = 1;         // in ms
 const int64_t kTimeSmallMax = 20000;     // in ms
 const uint32_t kTimeSmallBuckets = 100;
 
-// Converts a PP_FileHandle to a POSIX file descriptor.
-int32_t ConvertFileDescriptor(PP_FileHandle handle) {
-  PLUGIN_PRINTF(("ConvertFileDescriptor, handle=%d\n", handle));
-#if NACL_WINDOWS
-  int32_t file_desc = NACL_NO_FILE_DESC;
-  // On Windows, valid handles are 32 bit unsigned integers so this is safe.
-  file_desc = reinterpret_cast<intptr_t>(handle);
-  // Convert the Windows HANDLE from Pepper to a POSIX file descriptor.
-  int32_t posix_desc = _open_osfhandle(file_desc, _O_RDWR | _O_BINARY);
-  if (posix_desc == -1) {
-    // Close the Windows HANDLE if it can't be converted.
-    CloseHandle(reinterpret_cast<HANDLE>(file_desc));
-    return -1;
-  }
-  return posix_desc;
-#else
-  return handle;
-#endif
-}
-
-
 }  // namespace
 
 void Plugin::ShutDownSubprocesses() {
@@ -111,7 +90,7 @@ void Plugin::HistogramEnumerateSelLdrLoadStatus(NaClErrorCode error_code) {
 }
 
 bool Plugin::LoadNaClModuleFromBackgroundThread(
-    nacl::DescWrapper* wrapper,
+    PP_FileHandle file_handle,
     NaClSubprocess* subprocess,
     int32_t manifest_id,
     const SelLdrStartParams& params) {
@@ -143,9 +122,12 @@ bool Plugin::LoadNaClModuleFromBackgroundThread(
   PLUGIN_PRINTF(("Plugin::LoadNaClModuleFromBackgroundThread "
                  "(service_runtime_started=%d)\n",
                  service_runtime_started));
-  if (!service_runtime_started) {
+  if (!service_runtime_started)
     return false;
-  }
+
+  // TODO(teravest): Get rid of this conversion to DescWrapper.
+  int32_t fd = ConvertFileDescriptor(file_handle, true);
+  nacl::DescWrapper* wrapper = wrapper_factory()->MakeFileDesc(fd, O_RDONLY);
 
   // Now actually load the nexe, which can happen on a background thread.
   bool nexe_loaded = service_runtime->LoadNexeAndStart(
@@ -269,7 +251,7 @@ bool Plugin::LoadNaClModuleContinuationIntern() {
 }
 
 NaClSubprocess* Plugin::LoadHelperNaClModule(const nacl::string& helper_url,
-                                             nacl::DescWrapper* wrapper,
+                                             PP_FileHandle file_handle,
                                              int32_t manifest_id,
                                              ErrorInfo* error_info) {
   nacl::scoped_ptr<NaClSubprocess> nacl_subprocess(
@@ -296,7 +278,7 @@ NaClSubprocess* Plugin::LoadHelperNaClModule(const nacl::string& helper_url,
                            false /* enable_dyncode_syscalls */,
                            false /* enable_exception_handling */,
                            true /* enable_crash_throttling */);
-  if (!LoadNaClModuleFromBackgroundThread(wrapper, nacl_subprocess.get(),
+  if (!LoadNaClModuleFromBackgroundThread(file_handle, nacl_subprocess.get(),
                                           manifest_id, params)) {
     return NULL;
   }
@@ -435,7 +417,7 @@ void Plugin::NexeFileDidOpen(int32_t pp_error) {
   if (pp_error != PP_OK)
     return;
 
-  int32_t desc = ConvertFileDescriptor(nexe_handle_);
+  int32_t desc = ConvertFileDescriptor(nexe_handle_, true);
   nexe_handle_ = PP_kInvalidFileHandle;  // Clear out nexe handle.
 
   nacl::scoped_ptr<nacl::DescWrapper>

ppapi/native_client/src/trusted/plugin/plugin.h

@@ -105,12 +105,12 @@ class Plugin : public pp::Instance {
   bool LoadNaClModuleContinuation(int32_t pp_error);
 
   // Load support.
-  // A helper SRPC NaCl module can be loaded given a DescWrapper.
+  // A helper SRPC NaCl module can be loaded given a PP_FileHandle.
   // Blocks until the helper module signals initialization is done.
   // Does not update nacl_module_origin().
   // Returns NULL or the NaClSubprocess of the new helper NaCl module.
   NaClSubprocess* LoadHelperNaClModule(const nacl::string& helper_url,
-                                       nacl::DescWrapper* wrapper,
+                                       PP_FileHandle file_handle,
                                        int32_t manifest_id,
                                        ErrorInfo* error_info);
 
@@ -186,10 +186,10 @@ class Plugin : public pp::Instance {
   void HistogramEnumerateLoadStatus(PP_NaClError error_code);
   void HistogramEnumerateSelLdrLoadStatus(NaClErrorCode error_code);
 
-  // Load a nacl module from the file specified in wrapper.
+  // Load a nacl module from the file specified in file_handle.
   // Only to be used from a background (non-main) thread.
   // This will fully initialize the |subprocess| if the load was successful.
-  bool LoadNaClModuleFromBackgroundThread(nacl::DescWrapper* wrapper,
+  bool LoadNaClModuleFromBackgroundThread(PP_FileHandle file_handle,
                                           NaClSubprocess* subprocess,
                                           int32_t manifest_id,
                                           const SelLdrStartParams& params);

ppapi/native_client/src/trusted/plugin/pnacl_resources.cc

@@ -16,6 +16,15 @@
 
 namespace plugin {
 
+namespace {
+
+nacl::string GetFullUrl(const nacl::string& partial_url) {
+  return PnaclUrls::GetBaseUrl() + GetNaClInterface()->GetSandboxArch() + "/" +
+         partial_url;
+}
+
+}  // namespace
+
 static const char kPnaclBaseUrl[] = "chrome://pnacl-translator/";
 
 nacl::string PnaclUrls::GetBaseUrl() {
@@ -55,41 +64,10 @@ nacl::string PnaclUrls::GetResourceInfoUrl() {
 //////////////////////////////////////////////////////////////////////
 
 PnaclResources::~PnaclResources() {
-  for (std::map<nacl::string, nacl::DescWrapper*>::iterator
-           i = resource_wrappers_.begin(), e = resource_wrappers_.end();
-       i != e;
-       ++i) {
-    delete i->second;
-  }
-  resource_wrappers_.clear();
-}
-
-// static
-int32_t PnaclResources::GetPnaclFD(Plugin* plugin, const char* filename) {
-  PP_FileHandle file_handle =
-      plugin->nacl_interface()->GetReadonlyPnaclFd(filename);
-  if (file_handle == PP_kInvalidFileHandle)
-    return -1;
-
-#if NACL_WINDOWS
-  //////// Now try the posix view.
-  int32_t posix_desc = _open_osfhandle(reinterpret_cast<intptr_t>(file_handle),
-                                       _O_RDONLY | _O_BINARY);
-  if (posix_desc == -1) {
-    PLUGIN_PRINTF((
-        "PnaclResources::GetPnaclFD failed to convert HANDLE to posix\n"));
-    // Close the Windows HANDLE if it can't be converted.
-    CloseHandle(file_handle);
-  }
-  return posix_desc;
-#else
-  return file_handle;
-#endif
-}
-
-nacl::DescWrapper* PnaclResources::WrapperForUrl(const nacl::string& url) {
-  CHECK(resource_wrappers_.find(url) != resource_wrappers_.end());
-  return resource_wrappers_[url];
+  if (llc_file_handle_ != PP_kInvalidFileHandle)
+    CloseFileHandle(llc_file_handle_);
+  if (ld_file_handle_ != PP_kInvalidFileHandle)
+    CloseFileHandle(ld_file_handle_);
 }
 
 void PnaclResources::ReadResourceInfo(
@@ -117,53 +95,52 @@ void PnaclResources::ReadResourceInfo(
 
   pp::Var llc_tool_name(pp::PASS_REF, pp_llc_tool_name_var);
   pp::Var ld_tool_name(pp::PASS_REF, pp_ld_tool_name_var);
-  llc_tool_name_ = llc_tool_name.AsString();
-  ld_tool_name_ = ld_tool_name.AsString();
-  pp::Core* core = pp::Module::Get()->core();
-  core->CallOnMainThread(0, resource_info_read_cb, PP_OK);
+  llc_tool_name_ = GetFullUrl(llc_tool_name.AsString());
+  ld_tool_name_ = GetFullUrl(ld_tool_name.AsString());
+  pp::Module::Get()->core()->CallOnMainThread(0, resource_info_read_cb, PP_OK);
+}
+
+PP_FileHandle PnaclResources::TakeLlcFileHandle() {
+  PP_FileHandle to_return = llc_file_handle_;
+  llc_file_handle_ = PP_kInvalidFileHandle;
+  return to_return;
 }
 
-nacl::string PnaclResources::GetFullUrl(
-    const nacl::string& partial_url, const nacl::string& sandbox_arch) const {
-  return PnaclUrls::GetBaseUrl() + sandbox_arch + "/" + partial_url;
+PP_FileHandle PnaclResources::TakeLdFileHandle() {
+  PP_FileHandle to_return = ld_file_handle_;
+  ld_file_handle_ = PP_kInvalidFileHandle;
+  return to_return;
 }
 
 void PnaclResources::StartLoad(
     const pp::CompletionCallback& all_loaded_callback) {
   PLUGIN_PRINTF(("PnaclResources::StartLoad\n"));
 
-  std::vector<nacl::string> resource_urls;
-  resource_urls.push_back(llc_tool_name_);
-  resource_urls.push_back(ld_tool_name_);
-
-  PLUGIN_PRINTF(("PnaclResources::StartLoad -- local install of PNaCl.\n"));
   // Do a blocking load of each of the resources.
-  int32_t result = PP_OK;
-  for (size_t i = 0; i < resource_urls.size(); ++i) {
-    nacl::string full_url = GetFullUrl(
-        resource_urls[i], plugin_->nacl_interface()->GetSandboxArch());
-    nacl::string filename = PnaclUrls::PnaclComponentURLToFilename(full_url);
+  nacl::string llc_filename =
+      PnaclUrls::PnaclComponentURLToFilename(llc_tool_name_);
+  llc_file_handle_ =
+      plugin_->nacl_interface()->GetReadonlyPnaclFd(llc_filename.c_str());
+
+  nacl::string ld_filename =
+      PnaclUrls::PnaclComponentURLToFilename(ld_tool_name_);
+  ld_file_handle_ =
+      plugin_->nacl_interface()->GetReadonlyPnaclFd(ld_filename.c_str());
 
-    int32_t fd = PnaclResources::GetPnaclFD(plugin_, filename.c_str());
-    if (fd < 0) {
+  int32_t result = PP_OK;
+  if (llc_file_handle_ == PP_kInvalidFileHandle ||
+      ld_file_handle_ == PP_kInvalidFileHandle) {
     // File-open failed. Assume this means that the file is
     // not actually installed. This shouldn't actually occur since
-      // ReadResourceInfo() should happen first, and error out.
+    // ReadResourceInfo() fail first.
     coordinator_->ReportNonPpapiError(
         PP_NACL_ERROR_PNACL_RESOURCE_FETCH,
       nacl::string("The Portable Native Client (pnacl) component is not "
                    "installed. Please consult chrome://components for more "
                    "information."));
     result = PP_ERROR_FILENOTFOUND;
-      break;
-    } else {
-      resource_wrappers_[resource_urls[i]] =
-          plugin_->wrapper_factory()->MakeFileDesc(fd, O_RDONLY);
-    }
   }
-  // We're done!  Queue the callback.
-  pp::Core* core = pp::Module::Get()->core();
-  core->CallOnMainThread(0, all_loaded_callback, result);
+  pp::Module::Get()->core()->CallOnMainThread(0, all_loaded_callback, result);
 }
 
 }  // namespace plugin

ppapi/native_client/src/trusted/plugin/pnacl_resources.h

@@ -46,7 +46,9 @@ class PnaclResources {
   PnaclResources(Plugin* plugin,
                  PnaclCoordinator* coordinator)
       : plugin_(plugin),
-        coordinator_(coordinator) {
+        coordinator_(coordinator),
+        llc_file_handle_(PP_kInvalidFileHandle),
+        ld_file_handle_(PP_kInvalidFileHandle) {
   }
   virtual ~PnaclResources();
 
@@ -63,14 +65,8 @@ class PnaclResources {
   const nacl::string& GetLlcUrl() { return llc_tool_name_; }
   const nacl::string& GetLdUrl() { return ld_tool_name_; }
 
-  nacl::string GetFullUrl(const nacl::string& partial_url,
-                          const nacl::string& sandbox_arch) const;
-
-  // Get file descs by name. Only valid after StartLoad's completion callback
-  // fired.
-  nacl::DescWrapper* WrapperForUrl(const nacl::string& url);
-
-  static int32_t GetPnaclFD(Plugin* plugin, const char* filename);
+  PP_FileHandle TakeLlcFileHandle();
+  PP_FileHandle TakeLdFileHandle();
 
  private:
   NACL_DISALLOW_COPY_AND_ASSIGN(PnaclResources);
@@ -79,13 +75,16 @@ class PnaclResources {
   Plugin* plugin_;
   // The coordinator responsible for reporting errors, etc.
   PnaclCoordinator* coordinator_;
-  // The descriptor wrappers for the downloaded URLs.  Only valid
-  // once all_loaded_callback_ has been invoked.
-  std::map<nacl::string, nacl::DescWrapper*> resource_wrappers_;
 
   // Tool names for llc and ld; read from the resource info file.
   nacl::string llc_tool_name_;
   nacl::string ld_tool_name_;
+
+  // File handles for llc and ld executables, after they've been opened.
+  // Only valid after the callback for StartLoad() has been called, and until
+  // TakeLlcFileHandle()/TakeLdFileHandle() is called.
+  PP_FileHandle llc_file_handle_;
+  PP_FileHandle ld_file_handle_;
 };
 
 }  // namespace plugin;

ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc

@@ -146,29 +146,6 @@ void PnaclTranslateThread::PutBytes(std::vector<char>* bytes,
   bytes->resize(buffer_size);
 }
 
-NaClSubprocess* PnaclTranslateThread::StartSubprocess(
-    const nacl::string& url_for_nexe,
-    int32_t manifest_id,
-    ErrorInfo* error_info) {
-  PLUGIN_PRINTF(("PnaclTranslateThread::StartSubprocess (url_for_nexe=%s)\n",
-                 url_for_nexe.c_str()));
-  nacl::DescWrapper* wrapper = resources_->WrapperForUrl(url_for_nexe);
-  // Supply a URL for the translator components, different from the app URL,
-  // so that NaCl GDB can filter-out the translator processes (and not debug
-  // the translator itself). Must have a full URL with schema, otherwise the
-  // string gets silently dropped by GURL.
-  nacl::string full_url = resources_->GetFullUrl(
-      url_for_nexe, plugin_->nacl_interface()->GetSandboxArch());
-  nacl::scoped_ptr<NaClSubprocess> subprocess(plugin_->LoadHelperNaClModule(
-      full_url, wrapper, manifest_id, error_info));
-  if (subprocess.get() == NULL) {
-    PLUGIN_PRINTF((
-        "PnaclTranslateThread::StartSubprocess: subprocess creation failed\n"));
-    return NULL;
-  }
-  return subprocess.release();
-}
-
 void WINAPI PnaclTranslateThread::DoTranslateThread(void* arg) {
   PnaclTranslateThread* translator =
       reinterpret_cast<PnaclTranslateThread*>(arg);
@@ -180,25 +157,29 @@ void PnaclTranslateThread::DoTranslate() {
   SrpcParams params;
   std::vector<nacl::DescWrapper*> llc_out_files;
   size_t i;
-  for (i = 0; i < obj_files_->size(); i++) {
+  for (i = 0; i < obj_files_->size(); i++)
     llc_out_files.push_back((*obj_files_)[i]->write_wrapper());
-  }
-  for (; i < PnaclCoordinator::kMaxTranslatorObjectFiles; i++) {
+  for (; i < PnaclCoordinator::kMaxTranslatorObjectFiles; i++)
     llc_out_files.push_back(invalid_desc_wrapper_);
-  }
 
   pp::Core* core = pp::Module::Get()->core();
   {
     nacl::MutexLocker ml(&subprocess_mu_);
     int64_t llc_start_time = NaClGetTimeOfDayMicroseconds();
-    llc_subprocess_.reset(
-      StartSubprocess(resources_->GetLlcUrl(), manifest_id_, &error_info));
-    if (llc_subprocess_ == NULL) {
+    PP_FileHandle llc_file_handle = resources_->TakeLlcFileHandle();
+
+    // On success, ownership of llc_file_handle is transferred.
+    llc_subprocess_.reset(plugin_->LoadHelperNaClModule(
+        resources_->GetLlcUrl(), llc_file_handle, manifest_id_, &error_info));
+    if (llc_subprocess_.get() == NULL) {
+      if (llc_file_handle != PP_kInvalidFileHandle)
+        CloseFileHandle(llc_file_handle);
       TranslateFailed(PP_NACL_ERROR_PNACL_LLC_SETUP,
                       "Compile process could not be created: " +
                       error_info.message());
       return;
     }
+
     llc_subprocess_active_ = true;
     core->CallOnMainThread(0,
                            coordinator_->GetUMATimeCallback(
@@ -353,9 +334,8 @@ bool PnaclTranslateThread::RunLdSubprocess() {
     }
     ld_in_files.push_back((*obj_files_)[i]->read_wrapper());
   }
-  for (; i < PnaclCoordinator::kMaxTranslatorObjectFiles; i++) {
+  for (; i < PnaclCoordinator::kMaxTranslatorObjectFiles; i++)
     ld_in_files.push_back(invalid_desc_wrapper_);
-  }
 
   nacl::DescWrapper* ld_out_file = nexe_file_->write_wrapper();
   pp::Core* core = pp::Module::Get()->core();
@@ -363,9 +343,14 @@ bool PnaclTranslateThread::RunLdSubprocess() {
     // Create LD process
     nacl::MutexLocker ml(&subprocess_mu_);
     int64_t ld_start_time = NaClGetTimeOfDayMicroseconds();
-    ld_subprocess_.reset(
-      StartSubprocess(resources_->GetLdUrl(), manifest_id_, &error_info));
+    PP_FileHandle ld_file_handle = resources_->TakeLdFileHandle();
+
+    // On success, ownership of ld_file_handle is transferred.
+    ld_subprocess_.reset(plugin_->LoadHelperNaClModule(
+        resources_->GetLdUrl(), ld_file_handle, manifest_id_, &error_info));
     if (ld_subprocess_ == NULL) {
+      if (ld_file_handle != PP_kInvalidFileHandle)
+        CloseFileHandle(ld_file_handle);
       TranslateFailed(PP_NACL_ERROR_PNACL_LD_SETUP,
                       "Link process could not be created: " +
                       error_info.message());

ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h

@@ -67,10 +67,6 @@ class PnaclTranslateThread {
   int64_t GetCompileTime() const { return compile_time_; }
 
  private:
-  // Starts an individual llc or ld subprocess used for translation.
-  NaClSubprocess* StartSubprocess(const nacl::string& url,
-                                  int32_t manifest_id,
-                                  ErrorInfo* error_info);
   // Helper thread entry point for translation. Takes a pointer to
   // PnaclTranslateThread and calls DoTranslate().
   static void WINAPI DoTranslateThread(void* arg);

ppapi/native_client/src/trusted/plugin/service_runtime.cc

@@ -368,9 +368,12 @@ void PluginReverseInterface::OpenManifestEntry_MainThreadContinuation(
   if (PnaclUrls::IsPnaclComponent(mapped_url)) {
     // Special PNaCl support files, that are installed on the
     // user machine.
-    int32_t fd = PnaclResources::GetPnaclFD(
-        plugin_,
+    PP_FileHandle handle = plugin_->nacl_interface()->GetReadonlyPnaclFd(
         PnaclUrls::PnaclComponentURLToFilename(mapped_url).c_str());
+    int32_t fd = -1;
+    if (handle != PP_kInvalidFileHandle)
+      fd = ConvertFileDescriptor(handle, true);
+
     if (fd < 0) {
       // We checked earlier if the pnacl component wasn't installed
       // yet, so this shouldn't happen. At this point, we can't do much

ppapi/native_client/src/trusted/plugin/utility.cc

@@ -117,4 +117,34 @@ void SetNaClInterface(const PPB_NaCl_Private* nacl_interface) {
   g_nacl_interface = nacl_interface;
 }
 
+void CloseFileHandle(PP_FileHandle file_handle) {
+#if NACL_WINDOWS
+  CloseHandle(file_handle);
+#else
+  close(file_handle);
+#endif
+}
+
+// Converts a PP_FileHandle to a POSIX file descriptor.
+int32_t ConvertFileDescriptor(PP_FileHandle handle, bool read_only) {
+  PLUGIN_PRINTF(("ConvertFileDescriptor, handle=%d\n", handle));
+#if NACL_WINDOWS
+  int32_t file_desc = NACL_NO_FILE_DESC;
+  // On Windows, valid handles are 32 bit unsigned integers so this is safe.
+  file_desc = reinterpret_cast<intptr_t>(handle);
+  // Convert the Windows HANDLE from Pepper to a POSIX file descriptor.
+  int flags = _O_BINARY;
+  flags |= read_only ? _O_RDONLY : _O_RDWR;
+  int32_t posix_desc = _open_osfhandle(file_desc, flags);
+  if (posix_desc == -1) {
+    // Close the Windows HANDLE if it can't be converted.
+    CloseHandle(reinterpret_cast<HANDLE>(file_desc));
+    return -1;
+  }
+  return posix_desc;
+#else
+  return handle;
+#endif
+}
+
 }  // namespace plugin

ppapi/native_client/src/trusted/plugin/utility.h

@@ -13,6 +13,7 @@
 #include "native_client/src/include/portability.h"
 #include "native_client/src/shared/platform/nacl_threads.h"
 #include "native_client/src/shared/platform/nacl_time.h"
+#include "ppapi/c/private/pp_file_handle.h"
 #include "ppapi/c/private/ppb_nacl_private.h"
 
 #define SRPC_PLUGIN_DEBUG 1
@@ -30,6 +31,11 @@ bool IsValidIdentifierString(const char* strval, uint32_t* length);
 const PPB_NaCl_Private* GetNaClInterface();
 void SetNaClInterface(const PPB_NaCl_Private* nacl_interface);
 
+void CloseFileHandle(PP_FileHandle file_handle);
+
+// Converts a PP_FileHandle to a POSIX file descriptor.
+int32_t ConvertFileDescriptor(PP_FileHandle handle, bool read_only);
+
 // Debugging print utility
 extern int gNaClPluginDebugPrintEnabled;
 extern int NaClPluginPrintLog(const char *format, ...);