Support non-SFI mode in NaCl manifest file.

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

Index: ppapi/native_client/src/trusted/plugin/json_manifest.cc
diff --git a/ppapi/native_client/src/trusted/plugin/json_manifest.cc b/ppapi/native_client/src/trusted/plugin/json_manifest.cc
index be9b01b88c63425a2890b24ded776873f0a928d9..d1aac8658341703caa426fe4ad7623e3d66f41ff 100644
--- a/ppapi/native_client/src/trusted/plugin/json_manifest.cc
+++ b/ppapi/native_client/src/trusted/plugin/json_manifest.cc
@@ -32,8 +32,11 @@ const char* const kFilesKey =       "files";
 
 // ISA Dictionary keys
 const char* const kX8632Key =       "x86-32";
+const char* const kX8632NonSFIKey = "x86-32-nonsfi";
 const char* const kX8664Key =       "x86-64";
+const char* const kX8664NonSFIKey = "x86-64-nonsfi";
 const char* const kArmKey =         "arm";
+const char* const kArmNonSFIKey =   "arm-nonsfi";
 const char* const kPortableKey =    "portable";
 
 // Url Resolution keys
@@ -89,6 +92,11 @@ const char* const kOptLevelKey = "optlevel";
 //   }
 // }
 
+// Returns the key for the architecture in non-SFI mode.
+std::string GetNonSFIKey(const std::string& sandbox_isa) {
+  return sandbox_isa + "-nonsfi";
+}
+
 // Looks up |property_name| in the vector |valid_names| with length
 // |valid_name_count|.  Returns true if |property_name| is found.
 bool FindMatchingProperty(const nacl::string& property_name,
@@ -250,6 +258,7 @@ bool IsValidISADictionary(const Json::Value& dictionary,
                           const nacl::string& parent_key,
                           const nacl::string& sandbox_isa,
                           bool must_find_matching_entry,
+                          bool nonsfi_enabled,
                           ErrorInfo* error_info) {
   if (error_info == NULL) return false;
 
@@ -274,8 +283,11 @@ bool IsValidISADictionary(const Json::Value& dictionary,
     // The known values for NaCl ISA dictionaries in the manifest.
     static const char* kNaClManifestISAProperties[] = {
       kX8632Key,
+      kX8632NonSFIKey,
       kX8664Key,
+      kX8664NonSFIKey,
       kArmKey,
+      kArmNonSFIKey,
       // "portable" is here to allow checking that, if present, it can
       // only refer to an URL, such as for a data file, and not to
       // "pnacl-translate", which would cause the creation of a nexe.
@@ -343,9 +355,11 @@ bool IsValidISADictionary(const Json::Value& dictionary,
     // TODO(elijahtaylor) add ISA resolver here if we expand ISAs to include
     // micro-architectures that can resolve to multiple valid sandboxes.
     bool has_isa = dictionary.isMember(sandbox_isa);
+    bool has_nonsfi_isa =
+        nonsfi_enabled && dictionary.isMember(GetNonSFIKey(sandbox_isa));
     bool has_portable = dictionary.isMember(kPortableKey);
 
-    if (!has_isa && !has_portable) {
+    if (!has_isa && !has_nonsfi_isa && !has_portable) {
       error_info->SetReport(
           PP_NACL_ERROR_MANIFEST_PROGRAM_MISSING_ARCH,
           nacl::string("manifest: no version of ") + parent_key +
@@ -428,6 +442,7 @@ bool JsonManifest::MatchesSchema(ErrorInfo* error_info) {
                             kProgramKey,
                             sandbox_isa_,
                             true,
+                            nonsfi_enabled_,
                             error_info)) {
     return false;
   }
@@ -440,6 +455,7 @@ bool JsonManifest::MatchesSchema(ErrorInfo* error_info) {
                               kInterpreterKey,
                               sandbox_isa_,
                               true,
+                              nonsfi_enabled_,
                               error_info)) {
       return false;
     }
@@ -463,6 +479,7 @@ bool JsonManifest::MatchesSchema(ErrorInfo* error_info) {
                                 file_name,
                                 sandbox_isa_,
                                 false,
+                                nonsfi_enabled_,
                                 error_info)) {
         return false;
       }
@@ -476,13 +493,14 @@ bool JsonManifest::GetURLFromISADictionary(const Json::Value& dictionary,
                                            const nacl::string& parent_key,
                                            nacl::string* url,
                                            PnaclOptions* pnacl_options,
+                                           bool* uses_nonsfi_mode,
                                            ErrorInfo* error_info) const {
   DCHECK(url != NULL && pnacl_options != NULL && error_info != NULL);
 
   // When the application actually requests a resolved URL, we must have
   // a matching entry (sandbox_isa_ or portable) for NaCl.
   if (!IsValidISADictionary(dictionary, parent_key, sandbox_isa_, true,
-                            error_info)) {
+                            nonsfi_enabled_, error_info)) {
     error_info->SetReport(PP_NACL_ERROR_MANIFEST_RESOLVE_URL,
                           "architecture " + sandbox_isa_ +
                           " is not found for file " + parent_key);
@@ -495,11 +513,20 @@ bool JsonManifest::GetURLFromISADictionary(const Json::Value& dictionary,
   // sandbox_isa_ or kPortableKey is present in the dictionary.
   bool has_portable = dictionary.isMember(kPortableKey);
   bool has_isa = dictionary.isMember(sandbox_isa_);
+  // True iff the non-SFI mode is enabled, and the nmf as the -nonsfi entry.

[Mark Seaborn, 2014/03/04 04:03:32]

"as" -> "has".  But this comment echoes the code, so you could drop it.

[hidehiko, 2014/03/04 15:06:33]

Acknowledged. The code will gone.

+  bool has_nonsfi_isa =
+      nonsfi_enabled_ && dictionary.isMember(GetNonSFIKey(sandbox_isa_));

[Mark Seaborn, 2014/03/04 04:03:32]

If the manifest has "portable-nonsfi", would this return true?

[hidehiko, 2014/03/04 15:06:33]

Acknowledged. The code will gone.

   nacl::string chosen_isa;
-  if ((sandbox_isa_ == kPortableKey) || (has_portable && !has_isa)) {

[Mark Seaborn, 2014/03/04 04:03:32]

The existing logic seems convoluted, so I wonder if this can be simplified?  Why
doesn't this do:

if (dictionary.isMember(sandbox_isa_)) {
  chosen_isa = sandbox_isa_; // may be kPortableKey
} else if (dictionary.isMember(kPortableKey)) {
  chosen_isa = kPortableKey;
}

Then the logical extension for non-SFI mode is:

bool nonsfi_isa = GetNonSFIKey(sandbox_isa_);
if (sandbox_isa_ != kPortableKey &&
    nonsfi_enabled &&
    dictionary.isMember(nonsfi_isa)) {
  chosen_isa = nonsfi_isa;
} else if (dictionary.isMember(sandbox_isa_)) {
  chosen_isa = sandbox_isa_;  // May be kPortableKey
} else if (dictionary.isMember(kPortableKey)) {
  chosen_isa = kPortableKey;
}

This question is directed at Jan, who I'd like to have sign off on the
src/trusted/plugin/ change.

[hidehiko, 2014/03/04 15:06:33]

Thank you for comment.
Jan, WDYT?

I'll update the code after Jan's reply.

On 2014/03/04 04:03:32, Mark Seaborn wrote:

[jvoung (off chromium), 2014/03/04 16:24:18]

That looks okay. The main constraints are:

* PNaCl uses portable, and never uses anything else.
* NaCl will first try the sandbox isa, but falls back to portable if available.

I think the code was written to follow the structure of IsValidISADictionary(),
where it checks if sandbox_isa_ == kPortableKey first, and then branches...
except that it then deviated from the structure and folded in the case where
sandbox_isa_  != kPortableKey w/ "has_portable && !has_isa"

Anyway, Mark's suggestion is okay. Or if you want to make it more clear that
PNaCl can never use anything other than kPortableKey, you could do something
like:


if (sandbox_isa_ == kPortableKey) {
  chosen_isa = kPortableKey;
} else {
  if (nonsfi_enabled && dictionary.isMember(nonsfi_isa)) {
    chosen_isa = nonsfi_isa;
  } else if (dictionary.isMember(sandbox_isa_)) {
    chosen_isa = sandbox_isa_;
  } else if (dictionary.isMember(kPortableKey)) {
    chosen_isa = kPortableKey;
  } else {
    NOTREACHED();
  }
}

[hidehiko, 2014/03/05 08:15:33]

Thank you for comment. Done in Jan's way.

On 2014/03/04 16:24:18, jvoung (cr) wrote:

+  if ((sandbox_isa_ == kPortableKey) ||
+      (has_portable && !has_isa && !has_nonsfi_isa)) {
     chosen_isa = kPortableKey;
   } else {
-    chosen_isa = sandbox_isa_;
+    // Choose non-SFI iff available.
+    if (has_nonsfi_isa) {
+      chosen_isa = GetNonSFIKey(sandbox_isa_);
+    } else {
+      chosen_isa = sandbox_isa_;
+    }
   }
   const Json::Value& isa_spec = dictionary[chosen_isa];
   // Check if this requires a pnacl-translate, otherwise just grab the URL.
@@ -508,10 +535,12 @@ bool JsonManifest::GetURLFromISADictionary(const Json::Value& dictionary,
     // PNaCl
     GrabUrlAndPnaclOptions(isa_spec[kPnaclTranslateKey], url, pnacl_options);
     pnacl_options->set_translate(true);
+    *uses_nonsfi_mode = false;
   } else {
     // NaCl
     *url = isa_spec[kUrlKey].asString();
     pnacl_options->set_translate(false);
+    *uses_nonsfi_mode = has_nonsfi_isa;
   }
 
   return true;
@@ -530,8 +559,9 @@ bool JsonManifest::GetKeyUrl(const Json::Value& dictionary,
   }
   const Json::Value& isa_dict = dictionary[key];
   nacl::string relative_url;
+  bool uses_nonsfi_mode;
   if (!GetURLFromISADictionary(isa_dict, key, &relative_url,
-                               pnacl_options, error_info)) {
+                               pnacl_options, &uses_nonsfi_mode, error_info)) {
     return false;
   }
   return ResolveURL(relative_url, full_url, error_info);
@@ -559,11 +589,12 @@ bool JsonManifest::ResolveURL(const nacl::string& relative_url,
 
 bool JsonManifest::GetProgramURL(nacl::string* full_url,
                                  PnaclOptions* pnacl_options,
+                                 bool* uses_nonsfi_mode,
                                  ErrorInfo* error_info) const {
   if (full_url == NULL || pnacl_options == NULL || error_info == NULL)
     return false;
 
-  Json::Value program = dictionary_[kProgramKey];
+  const Json::Value& program = dictionary_[kProgramKey];
 
   nacl::string nexe_url;
   nacl::string error_string;
@@ -572,6 +603,7 @@ bool JsonManifest::GetProgramURL(nacl::string* full_url,
                                kProgramKey,
                                &nexe_url,
                                pnacl_options,
+                               uses_nonsfi_mode,
                                error_info)) {
     return false;
   }