Change the pnacl shim from doing real shimming to just intercepting

src/untrusted/pnacl_irt_shim/shim_entry.c

 /*
- * Copyright (c) 2011 The Native Client Authors. All rights reserved.
+ * Copyright 2011 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
+#include <stddef.h>

[Mark Seaborn, 2012/08/28 16:04:51]

I don't think you actually need stddef.h below, do you?

[Robert Muth (chromium), 2012/08/28 19:11:24]

irt.h depends on it.

[Mark Seaborn, 2012/08/28 19:38:41]

But irt.h contains "#include <stddef.h>".  So I don't think you need this
#include.

 #include "native_client/src/include/elf32.h"
 #include "native_client/src/include/elf_auxv.h"
 #include "native_client/src/include/nacl_macros.h"
-#include "native_client/src/untrusted/pnacl_irt_shim/shim_ppapi.h"
+#include "native_client/src/untrusted/irt/irt.h"
 #include "native_client/src/untrusted/nacl/nacl_startup.h"
 
+/*
+ * For more information about this hack cf.
+ * src/untrusted/irt/irt_ppapi.c
+ */
+
+static TYPE_nacl_irt_query real_irt_interface;
+
+/* cf. src/untrusted/irt/irt.h  NACL_IRT_PPAPIHOOK_(SHIMMED_)v0_1 */
+static const char prefix_search[] = "nacl-irt-ppapihook";
+static const char prefix_replace[] = "nacl-irt-ppapihook-shimmed";
+
+/* do not make assumptions about stcmp being available */

[Mark Seaborn, 2012/08/28 16:04:51]

'stcmp' -> 'strcmp'.  Also capitalise the first letter of the sentence, please.

[Robert Muth (chromium), 2012/08/28 19:11:24]

Done.

+static int my_strcmp(const char* s1, const char* s2) {

[Mark Seaborn, 2012/08/28 16:04:51]

This should use the " *" spacing style (as elsewhere in the file) and not "* ".

[Robert Muth (chromium), 2012/08/28 19:11:24]

Done.

+  while( *s1 != '\0' && *s2 != '\0' && *s1 == *s2) {

[Mark Seaborn, 2012/08/28 16:04:51]

"while( " -> "while ("

[Robert Muth (chromium), 2012/08/28 19:11:24]

Done.

+    s1++;
+    s2++;
+  }
+  return *s1 - *s2;
+}
+
+/* do not make assumptions about strcpy being available */
+static void my_strcpy(char* s1, const char* s2) {
+  while(*s2 != '\0') {
+    *s1 = *s2;
+    s1++;
+    s2++;
+  }
+  *s1 = '\0';
+}
+
+static size_t pnacl_irt_interface_interceptor(const char *interface_ident,
+                                              void *table, size_t tablesize) {
+  /* make this big enough to hold prefix_replace + version suffix */
+  char buffer[2 * sizeof(prefix_replace)];
+
+  const char* ident = interface_ident;
+  /* rewrite: "nacl-irt-ppapihook-XXX" -> "nacl-irt-ppapihook-shimmed-XXX" */
+  if (0 == my_strcmp(interface_ident, prefix_search)) {

[Mark Seaborn, 2012/08/28 16:04:51]

When is this check going to return true, if interface_ident contains a full
interface name and prefix_search only contains an interface name prefix?  How
have you tested this code?

Anyway, checking for an interface name prefix is unnecessary complexity.  Please
just check for the full interface name, NACL_IRT_PPAPIHOOK_v0_1.  The likelihood
that we will need to change the NACL_IRT_PPAPIHOOK interface in the future is
very small.

[Robert Muth (chromium), 2012/08/28 19:11:24]

Thanks this was a bug.
As I mention in the initial email - this code has not been fully tested yet.
ok specialized this to the current interface.

[Mark Seaborn, 2012/08/28 19:38:41]

Why don't you combine this change into your IRT change,
http://codereview.chromium.org/10826171/?  It would make sense to introduce the
new IRT interface in the same change as you add code that uses it.

+    /* but not if it is already  "nacl-irt-ppapihook-shimmed-XXX" */
+    if (0 != my_strcmp(interface_ident, prefix_replace)) {
+      my_strcpy(buffer, prefix_replace);
+      my_strcpy(buffer + sizeof(prefix_replace) - 1,
+               interface_ident + sizeof(prefix_search) - 1);
+      ident = buffer;
+    }
+  }
+  return real_irt_interface(ident, table, tablesize);
+}
 
 /*
  * This is the true entry point for untrusted code.
  * See nacl_startup.h for the layout at the argument pointer.
  */
 void _pnacl_wrapper_start(uint32_t *info) {
   Elf32_auxv_t *auxv = nacl_startup_auxv(info);
 
   Elf32_auxv_t *entry = NULL;
   for (Elf32_auxv_t *av = auxv; av->a_type != AT_NULL; ++av) {
     if (av->a_type == AT_SYSINFO) {
       entry = av;
       break;
     }
   }
 
   if (entry != NULL) {
     /*
      * Save the real irt interface.
      */
-    __pnacl_real_irt_interface = (TYPE_nacl_irt_query) entry->a_un.a_val;
+    real_irt_interface = (TYPE_nacl_irt_query) entry->a_un.a_val;
 
     /*
      * Overwrite the auxv slot with the pnacl IRT shim query function.
      */
     entry->a_type = AT_SYSINFO;
-    entry->a_un.a_val = (uintptr_t) __pnacl_irt_interface_wrapper;
+    entry->a_un.a_val = (uintptr_t) pnacl_irt_interface_interceptor;
   }
 
   /* If entry is NULL still allow startup to continue.  It may be the case
    * that the IRT was not actually used (e.g., for some commandline tests).
    * For newlib, we can tell that the IRT isn't used when libnacl_sys_private.a
    * is in the bitcode link line. However, glibc does not use
    * libnacl_sys_private, so that would not work. We could look for -lppapi
    * in the bitcode link line, but looking at the bitcode link line
    * seems brittle (what if the bitcode link was separated from translation).
    * Thus we always wrap _start, even if there is no IRT auxv entry.
    */
 
   /*
    * Call the user entry point function.  It should not return.
    * TODO(sehr): Find a way to ensure this is invoked via a tail call.
    */
   _start(info);
 }