this patch adds the manifest proxy server to sel_ldr and the manifest

src/trusted/service_runtime/sel_ldr_standard.c

Index: src/trusted/service_runtime/sel_ldr_standard.c
===================================================================
--- src/trusted/service_runtime/sel_ldr_standard.c	(revision 5616)
+++ src/trusted/service_runtime/sel_ldr_standard.c	(working copy)
@@ -24,7 +24,10 @@
 #include "native_client/src/shared/platform/nacl_time.h"
 #include "native_client/src/trusted/perf_counter/nacl_perf_counter.h"
 
+#include "native_client/src/trusted/manifest_name_service_proxy/manifest_proxy.h"

[noelallen_use_chromium, 2011/06/14 02:25:45]

include order?

[bsy, 2011/06/14 20:30:03]

Done.

+
 #include "native_client/src/trusted/service_runtime/include/sys/errno.h"
+#include "native_client/src/trusted/service_runtime/include/sys/fcntl.h"
 
 #include "native_client/src/trusted/service_runtime/arch/sel_ldr_arch.h"
 #include "native_client/src/trusted/service_runtime/elf_util.h"
@@ -37,6 +40,7 @@
 #include "native_client/src/trusted/service_runtime/outer_sandbox.h"
 #include "native_client/src/trusted/service_runtime/sel_memory.h"
 #include "native_client/src/trusted/service_runtime/sel_ldr.h"
+#include "native_client/src/trusted/service_runtime/sel_ldr_thread_interface.h"
 #include "native_client/src/trusted/service_runtime/sel_util.h"
 #include "native_client/src/trusted/service_runtime/sel_addrspace.h"
 
@@ -522,8 +526,82 @@
 }
 
 int NaClAppLaunchServiceThreads(struct NaClApp *nap) {
+  struct NaClManifestProxy  *manifest_proxy = NULL;
+  int                       rv;
+
   NaClNameServiceLaunch(nap->name_service);
-  return 1;
+
+  NaClXMutexLock(&nap->mu);

[noelallen_use_chromium, 2011/06/14 02:25:45]

Comment?
As is, this code appears racy...  The variable could have been setup between the
unlock and the if.   I suspect the only reason you are locking this at all is to
prevent a thread sanitizer warning?

[bsy, 2011/06/14 20:30:03]

reverse_channel_initialized is set in the RPC handler for reverse_setup call --
which occurs in the secure command channel srpc handler thread.  thus access to
it should be locked, or else cache coherency issues might cause us to see it as
not initialized when it really was initialized.

yes, the RPCs needed to launch the app is defined so that the plugin must do
reverse_setup before start_module, so that by the time this fn is invoked,
reverse_channel_initialized will have long be set -- and there would have been
enough other synchronization operations that an unlocked read would still yield
the correct value.  i don't know for sure whether tsan would warn since i
haven't run it, but i suspect it would (it should!).

i'll add a comment.

+  rv = !nap->reverse_channel_initialized;
+  NaClXMutexUnlock(&nap->mu);
+  if (rv) {
+    NaClLog(3,
+            ("NaClAppLaunchServiceThreads: no reverse channel;"
+             " NOT launching manifest proxy\n"));
+    goto done;
+  }
+
+  rv = 0;
+  /*
+   * Allocate/construct the manifest proxy without grabbing global
+   * locks.
+   */
+  NaClLog(3, "NaClAppLaunchServiceThreads: launching manifest proxy\n");
+
+  /*
+   * ReverseClientSetup RPC should be done via the command channel
+   * prior to the load_module / start_module RPCs, and
+   *  occurs after that, so checking
+   * nap->reverse_client suffices for determining whether the proxy is
+   * exporting reverse services.
+   */
+  manifest_proxy = (struct NaClManifestProxy *) malloc(sizeof *manifest_proxy);
+  if (NULL == manifest_proxy) {
+    NaClLog(LOG_ERROR, "No memory for manifest proxy\n");
+    goto manifest_proxy_alloc_failure;
+  }
+  if (!NaClManifestProxyCtor(manifest_proxy,
+                             NaClAddrSpSquattingThreadIfFactoryFunction,
+                             (void *) nap,
+                             nap)) {
+    NaClLog(LOG_ERROR, "ManifestProxyCtor failed\n");
+    goto manifest_proxy_ctor_failure;
+  }
+
+  /*
+   * StartThread requires lock.

[noelallen_use_chromium, 2011/06/14 02:25:45]

Where is the lock this comment mentions?

[bsy, 2011/06/14 20:30:03]

clarified.

+   */
+  if (!NaClSimpleServiceStartServiceThread((struct NaClSimpleService *)
+                                           manifest_proxy)) {
+    NaClLog(LOG_ERROR, "ManifestProxy start service failed\n");
+    NaClRefCountUnref((struct NaClRefCount *) manifest_proxy);

[noelallen_use_chromium, 2011/06/14 02:25:45]

This is counter-intuitive.  Internally NaClSimpleServiceStartServiceThread does
a refcnt even on failure?  If it failed shouldn't we be freeing this object
instead of unrefing it?

[bsy, 2011/06/14 20:30:03]

any object of a NaClRefCount subclass, after it's been fully constructed, is
freed only as a side effect of decrementing its refcount (which also runs the
dtor chain before freeing).  the manifest_proxy is fully constructed at this
point.

+    manifest_proxy = NULL;
+    goto manifest_proxy_start_failed;
+  }
+
+  NaClXMutexLock(&nap->mu);
+  CHECK(NULL == nap->manifest_proxy);
+
+  nap->manifest_proxy = manifest_proxy;
+  manifest_proxy = NULL;
+
+  NaClLog(3,
+          ("NaClAppLaunchServiceThreads: adding manifest proxy to"
+           " name service\n"));
+  (*NACL_VTBL(NaClNameService, nap->name_service)->
+   CreateDescEntry)(nap->name_service,
+                    "manifest_proxy", NACL_ABI_O_RDWR,
+                    NaClDescRef(nap->manifest_proxy->base.bound_and_cap[1]));
+
+  rv = 1;
+  NaClXMutexUnlock(&nap->mu);
+
+manifest_proxy_start_failed:
+manifest_proxy_ctor_failure:
+  free(manifest_proxy);
+manifest_proxy_alloc_failure:
+done:
+  return rv;
 }
 
 /*