From c9a452c73eca20f45349f816bf4fd96627438d95 Mon Sep 17 00:00:00 2001
From: Leo Osvald <leoo@fb.com>
Date: Thu, 5 Mar 2020 15:42:18 -0800
Subject: [PATCH] Parse HHBC option as bool only if it's a flag

Summary:
The JSON config passed to hh_single_compile by HHVM contains a
**superset** of HHBC-relevant config, e.g.:

  { ...
    "hhvm.trusted_db_path": {
       "access": 4,
       "local_value": "",
       "global_value": "" }
  }

Therefore, skip the interpretation of such an option as flag in Rust
**until *after*** it's determined that it indeed represents a flag.
Now, unrelated HHVM options with values such as:

- "" (empty string); or
- "12345678901234567890" (integer-overflowing constant)

no longer panic / make the whole JSON parse to fail
(as a result of parsing a non-int/bool-convertible string).

Add regression tests for these two cases.

Finally, treat empty string values as false for flags as in OCaml code.

Reviewed By: dabek

Differential Revision: D20265758

fbshipit-source-id: 74cac1d8a718c35a06a804aee5f9a97268f3ef30
---
 hphp/hack/src/hhbc/options.rs | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/hphp/hack/src/hhbc/options.rs b/hphp/hack/src/hhbc/options.rs
index a6daa892832..a457eff1473 100644
--- a/hphp/hack/src/hhbc/options.rs
+++ b/hphp/hack/src/hhbc/options.rs
@@ -499,21 +499,25 @@ fn deserialize_flags<'de, D: Deserializer<'de>, P: PrefixedFlags>(
                 num => num.parse::<i32>().map(|v| v > 0).map_err(de::Error::custom),
             };
             while let Some((ref k, ref v)) = map.next_entry::<String, Arg<GlobalValue>>()? {
-                let truish = match v.get() {
-                    GlobalValue::String(s) => from_str(s)?,
-                    GlobalValue::Bool(b) => *b,
-                    GlobalValue::Int(n) => *n != 0,
-                    _ => continue, // types such as VecStr aren't parsable as flags
-                };
-                // println!("k={:?} v={:?}~{} flag={:?}", k, v, truish, by_name.get(k));
                 let mut found = None;
                 let k: &str = &k;
                 let k: &str = options_cli::CANON_BY_ALIAS.get(k).unwrap_or(&k);
                 if k.starts_with(P::PREFIX) {
                     found = by_name.get(&k[prefix_len..]).map(|p| *p);
                 }
-
                 if let Some(flag) = found {
+                    let truish = match v.get() {
+                        GlobalValue::String(s) => {
+                            if s.is_empty() {
+                                false
+                            } else {
+                                from_str(s)?
+                            }
+                        }
+                        GlobalValue::Bool(b) => *b,
+                        GlobalValue::Int(n) => *n != 0,
+                        _ => continue, // types such as VecStr aren't parsable as flags
+                    };
                     if truish {
                         flags |= flag
                     } else {
@@ -752,6 +756,15 @@ mod tests {
     }
 
     #[test]
+    fn test_empty_flag_treated_as_false_json_de() {
+        // verify a true-by-default flag was parsed as false if ""
+        let hhvm: Hhvm =
+            serde_json::from_str(r#"{ "hhvm.emit_func_pointers": { "global_value": "" } }"#)
+                .unwrap();
+        assert!(!hhvm.flags.contains(HhvmFlags::EMIT_FUNC_POINTERS));
+    }
+
+    #[test]
     fn test_options_flat_arg_alias_json_de() {
         let act: Options = serde_json::value::from_value(json!({
             "eval.jitenablerenamefunction": {
@@ -940,6 +953,17 @@ mod tests {
     }
 
     #[test]
+    fn test_options_de_regression_boolish_parse_on_unrelated_opt() {
+        // Note: this fails if bool-looking options are too eagerly parsed
+        // (i.e., before they're are looked by JSON key/name and match a flag)
+        let _: Options = serde_json::value::from_value(json!({
+            "hhvm.only.opt1": { "global_value": "12345678901234567890" },
+             "hhvm.only.opt2": { "global_value": "" },
+        }))
+        .expect("boolish-parsing logic wrongly triggered");
+    }
+
+    #[test]
     fn test_major_outlier_source_mapping_serde() {
         use serde::ser::Serialize;
         fn mk_source_mapping<T: Serialize>(v: T) -> Json {
-- 
2.11.4.GIT