Backed out 4 changesets (bug 1733686, bug 1735935) for causing sandbo…

…xing crashes with the spellchecker (bug 1736171) . CLOSED TREE

Backed out changeset c981fa4490fe (bug 1735935)
Backed out changeset a6fef4dc35c2 (bug 1733686)
Backed out changeset d52827e69092 (bug 1733686)
Backed out changeset 29ed3620fa91 (bug 1733686)

config/external/rlbox/rlbox_config.h

@@ -6,8 +6,6 @@
 #ifndef RLBOX_CONFIG
 #define RLBOX_CONFIG
 
-#include "mozilla/Assertions.h"
-
 #ifdef XP_MACOSX
 
 // RLBox uses c++17's shared_locks by default, even for the noop_sandbox
@@ -33,8 +31,6 @@ struct rlbox_shared_lock {
 // performance
 #define RLBOX_SINGLE_THREADED_INVOCATIONS
 
-#define RLBOX_CUSTOM_ABORT(msg) MOZ_CRASH_UNSAFE_PRINTF("RLBox crash: %s", msg)
-
 // The MingW compiler does not correctly handle static thread_local inline
 // members. This toggles a workaround that allows the host application (firefox)
 // to provide TLS storage via functions. This can be removed if the MingW bug is

extensions/spellcheck/hunspell/glue/RLBoxHunspell.cpp

@@ -19,9 +19,8 @@ static tainted_hunspell<char*> allocStrInSandbox(
     rlbox_sandbox_hunspell& aSandbox, const nsAutoCString& str) {
   size_t size = str.Length() + 1;
   tainted_hunspell<char*> t_str = aSandbox.malloc_in_sandbox<char>(size);
-  if (t_str) {
-    rlbox::memcpy(aSandbox, t_str, str.get(), size);
-  }
+  MOZ_RELEASE_ASSERT(t_str);
+  rlbox::memcpy(aSandbox, t_str, str.get(), size);
   return t_str;
 }
 
@@ -30,9 +29,8 @@ static tainted_hunspell<char*> allocStrInSandbox(
     rlbox_sandbox_hunspell& aSandbox, const std::string& str) {
   size_t size = str.size() + 1;
   tainted_hunspell<char*> t_str = aSandbox.malloc_in_sandbox<char>(size);
-  if (t_str) {
-    rlbox::memcpy(aSandbox, t_str, str.c_str(), size);
-  }
+  MOZ_RELEASE_ASSERT(t_str);
+  rlbox::memcpy(aSandbox, t_str, str.c_str(), size);
   return t_str;
 }
 
@@ -73,13 +71,8 @@ RLBoxHunspell::RLBoxHunspell(const nsAutoCString& affpath,
                                    mHunspellGetCurrentCS);
 
   // Copy the affpath and dpath into the sandbox
-  // These allocations should definitely succeed as these are first allocations
-  // inside the sandbox.
   tainted_hunspell<char*> t_affpath = allocStrInSandbox(mSandbox, affpath);
-  MOZ_RELEASE_ASSERT(t_affpath);
-
   tainted_hunspell<char*> t_dpath = allocStrInSandbox(mSandbox, dpath);
-  MOZ_RELEASE_ASSERT(t_dpath);
 
   // Create handle
   mHandle = mSandbox.invoke_sandbox_function(
@@ -123,12 +116,6 @@ RLBoxHunspell::~RLBoxHunspell() {
 int RLBoxHunspell::spell(const std::string& stdWord) {
   // Copy word into the sandbox
   tainted_hunspell<char*> t_word = allocStrInSandbox(mSandbox, stdWord);
-  if (!t_word) {
-    // Ran out of memory in the hunspell sandbox
-    // Fail gracefully assuming the word is spelt correctly
-    const int ok = 1;
-    return ok;
-  }
 
   // Check word
   int good = mSandbox
@@ -144,23 +131,12 @@ const std::string& RLBoxHunspell::get_dict_encoding() const {
   return mDicEncoding;
 }
 
-// This function fails gracefully - if we run out of memory in the hunspell
-// sandbox, we return empty suggestion list
 std::vector<std::string> RLBoxHunspell::suggest(const std::string& stdWord) {
   // Copy word into the sandbox
   tainted_hunspell<char*> t_word = allocStrInSandbox(mSandbox, stdWord);
-  if (!t_word) {
-    return {};
-  }
 
   // Allocate suggestion list in the sandbox
   tainted_hunspell<char***> t_slst = mSandbox.malloc_in_sandbox<char**>();
-  if (!t_slst) {
-    // Free the earlier allocation
-    mSandbox.free_in_sandbox(t_word);
-    return {};
-  }
-
   *t_slst = nullptr;
 
   // Get suggestions
@@ -173,26 +149,16 @@ std::vector<std::string> RLBoxHunspell::suggest(const std::string& stdWord) {
                  return nr;
                });
 
-  tainted_hunspell<char**> t_slst_ref = *t_slst;
-
+  // Copy suggestions from sandbox
   std::vector<std::string> suggestions;
-  if (nr > 0 && t_slst_ref != nullptr) {
-    // Copy suggestions from sandbox
-    suggestions.reserve(nr);
-
-    for (int i = 0; i < nr; i++) {
-      tainted_hunspell<char*> t_sug = t_slst_ref[i];
-
-      if (t_sug) {
-        t_sug.copy_and_verify_string(
-            [&](std::string sug) { suggestions.push_back(std::move(sug)); });
-        // free the suggestion string allocated by the sandboxed hunspell
-        mSandbox.free_in_sandbox(t_sug);
-      }
-    }
-
-    // free the suggestion list allocated by the sandboxed hunspell
-    mSandbox.free_in_sandbox(t_slst_ref);
+  suggestions.reserve(nr);
+  for (int i = 0; i < nr; i++) {
+    tainted_hunspell<char*> t_sug = (*t_slst)[i];
+    MOZ_RELEASE_ASSERT(t_sug);
+    t_sug.copy_and_verify_string([&](std::unique_ptr<char[]> sug) {
+      size_t len = std::strlen(sug.get());
+      suggestions.push_back(std::string(sug.get(), len));
+    });
   }
 
   mSandbox.free_in_sandbox(t_word);

extensions/spellcheck/hunspell/glue/mozHunspellRLBoxHost.cpp

@@ -146,19 +146,12 @@ tainted_hunspell<bool> mozHunspellCallbacks::GetLine(
       mozHunspellCallbacks::GetMozHunspellFileMgrHost(t_aFd);
   std::string line;
   bool ok = inst.GetLine(line);
-  // If the getline fails, return a null which is "graceful" failure
   if (ok) {
-    // Copy the line into the sandbox. This memory is eventually freed by
-    // hunspell.
+    // copy the line into the sandbox
     size_t size = line.size() + 1;
     tainted_hunspell<char*> t_line = aSandbox.malloc_in_sandbox<char>(size);
-
-    if (t_line == nullptr) {
-      // If malloc fails, we should go to "graceful" failure path
-      ok = false;
-    } else {
-      rlbox::memcpy(aSandbox, t_line, line.c_str(), size);
-    }
+    MOZ_RELEASE_ASSERT(t_line);
+    rlbox::memcpy(aSandbox, t_line, line.c_str(), size);
     *t_aLinePtr = t_line;
   } else {
     *t_aLinePtr = nullptr;

third_party/rlbox/README-mozilla

@@ -1,7 +1,7 @@
 This directory contains the rlbox source from the upstream repo:
 https://github.com/PLSysSec/rlbox_sandboxing_api/
 
-Current version: [commit a441977c8ff2da6626a3ff1a3cd2c6d0a9a9bed5]
+Current version: [commit fc796e549b3a48e89b9a8db28011dcad06494ba3]
 
 UPDATING:
 

third_party/rlbox/include/rlbox.hpp

@@ -655,7 +655,7 @@ class tainted_base_impl
    * @brief Copy a tainted string from sandbox and verify it.
    *
    * @param verifer Function used to verify the copied value.
-   * @tparam T_Func the type of the verifier either ``T_Ret(*)(unique_ptr<char[]>)`` or ``T_Ret(*)(std::string)``
+   * @tparam T_Func the type of the verifier ``T_Ret(*)(unique_ptr<char[]>)``
    * @return Whatever the verifier function returns.
    */
   template<typename T_Func>
@@ -667,53 +667,23 @@ class tainted_base_impl
     static_assert(std::is_same_v<char, T_CopyAndVerifyRangeEl>,
                   "copy_and_verify_string only allows char*");
 
-    using T_VerifParam = detail::func_first_arg_t<T_Func>;
-
     auto start = impl().get_raw_value();
-    if_constexpr_named(cond1, std::is_same_v<T_VerifParam, std::unique_ptr<char[]>> || std::is_same_v<T_VerifParam, std::unique_ptr<const char[]>>) {
-      if (start == nullptr) {
-        return verifier(nullptr);
-      }
+    if (start == nullptr) {
+      return verifier(nullptr);
+    }
 
-      // it is safe to run strlen on a tainted<string> as worst case, the string
-      // does not have a null and we try to copy all the memory out of the sandbox
-      // however, copy_and_verify_range ensures that we never copy memory outsider
-      // the range
-      auto str_len = std::strlen(start) + 1;
-      std::unique_ptr<T_CopyAndVerifyRangeEl[]> target =
-        copy_and_verify_range_helper(str_len);
-
-      // ensure the string has a trailing null
-      target[str_len - 1] = '\0';
-
-      return verifier(std::move(target));
-    } else if_constexpr_named (cond2, std::is_same_v<T_VerifParam, std::string>) {
-      if (start == nullptr) {
-        std::string param = "";
-        return verifier(param);
-      }
+    // it is safe to run strlen on a tainted<string> as worst case, the string
+    // does not have a null and we try to copy all the memory out of the sandbox
+    // however, copy_and_verify_range ensures that we never copy memory outsider
+    // the range
+    auto str_len = std::strlen(start) + 1;
+    std::unique_ptr<T_CopyAndVerifyRangeEl[]> target =
+      copy_and_verify_range_helper(str_len);
 
-      // it is safe to run strlen on a tainted<string> as worst case, the string
-      // does not have a null and we try to copy all the memory out of the sandbox
-      // however, copy_and_verify_range ensures that we never copy memory outsider
-      // the range
-      auto str_len = std::strlen(start) + 1;
+    // ensure the string has a trailing null
+    target[str_len - 1] = '\0';
 
-      const char* checked_start = (const char*) verify_range_helper(str_len);
-      if (checked_start == nullptr) {
-        std::string param = "";
-        return verifier(param);
-      }
-
-      std::string copy(checked_start, str_len - 1);
-      return verifier(std::move(copy));
-    } else {
-      constexpr bool unknownCase = !(cond1 || cond2);
-      rlbox_detail_static_fail_because(
-        unknownCase,
-        "copy_and_verify_string verifier parameter should either be unique_ptr<char[]>, unique_ptr<const char[]> or std::string"
-      );
-    }
+    return verifier(std::move(target));
   }
 
   /**
@@ -1182,7 +1152,6 @@ class tainted_volatile : public tainted_base_impl<tainted_volatile, T, T_Sbx>
   inline std::remove_cv_t<T_SandboxedType> get_raw_sandbox_value(
     rlbox_sandbox<T_Sbx>& sandbox) const noexcept
   {
-    RLBOX_UNUSED(sandbox);
     return data;
   };
 
@@ -1200,7 +1169,6 @@ class tainted_volatile : public tainted_base_impl<tainted_volatile, T, T_Sbx>
   inline std::remove_cv_t<T_SandboxedType> get_raw_sandbox_value(
     rlbox_sandbox<T_Sbx>& sandbox) noexcept
   {
-    RLBOX_UNUSED(sandbox);
     rlbox_detail_forward_to_const(get_raw_sandbox_value,
                                   std::remove_cv_t<T_SandboxedType>);
   };
@@ -1293,7 +1261,7 @@ class tainted_volatile : public tainted_base_impl<tainted_volatile, T, T_Sbx>
         // is safe.
         auto func = val.get_raw_sandbox_value();
         using T_Cast = std::remove_volatile_t<T_SandboxedType>;
-        get_sandbox_value_ref() = (T_Cast)func;
+        get_sandbox_value_ref() = reinterpret_cast<T_Cast>(func);
       }
     }
     else if_constexpr_named(

third_party/rlbox/include/rlbox_app_pointer.hpp

@@ -32,13 +32,13 @@ class app_pointer_map
     for (T_PointerTypeUnsigned i = counter; i < max_val; i++) {
       if (pointer_map.find(i) == pointer_map.end()) {
         counter = i + 1;
-        return (T_PointerType)i;
+        return reinterpret_cast<T_PointerType>(i);
       }
     }
     for (T_PointerTypeUnsigned i = min_val; i < counter; i++) {
       if (pointer_map.find(i) == pointer_map.end()) {
         counter = i + 1;
-        return (T_PointerType)i;
+        return reinterpret_cast<T_PointerType>(i);
       }
     }
     detail::dynamic_check(false, "Could not find free app pointer slot");
@@ -57,15 +57,17 @@ class app_pointer_map
   {
     RLBOX_ACQUIRE_UNIQUE_GUARD(lock, map_mutex);
     T_PointerType idx = get_unused_index();
-    T_PointerTypeUnsigned idx_int = (T_PointerTypeUnsigned)idx;
+    T_PointerTypeUnsigned idx_int =
+      reinterpret_cast<T_PointerTypeUnsigned>(idx);
     pointer_map[idx_int] = ptr;
     return idx;
   }
 
   void remove_app_ptr(T_PointerType idx)
   {
     RLBOX_ACQUIRE_UNIQUE_GUARD(lock, map_mutex);
-    T_PointerTypeUnsigned idx_int = (T_PointerTypeUnsigned)idx;
+    T_PointerTypeUnsigned idx_int =
+      reinterpret_cast<T_PointerTypeUnsigned>(idx);
     auto it = pointer_map.find(idx_int);
     detail::dynamic_check(it != pointer_map.end(),
                           "Error: removing a non-existing app pointer");
@@ -75,7 +77,8 @@ class app_pointer_map
   void* lookup_index(T_PointerType idx)
   {
     RLBOX_ACQUIRE_SHARED_GUARD(lock, map_mutex);
-    T_PointerTypeUnsigned idx_int = (T_PointerTypeUnsigned)idx;
+    T_PointerTypeUnsigned idx_int =
+      reinterpret_cast<T_PointerTypeUnsigned>(idx);
     auto it = pointer_map.find(idx_int);
     detail::dynamic_check(it != pointer_map.end(),
                           "Error: looking up a non-existing app pointer");

third_party/rlbox/include/rlbox_conversion.hpp

@@ -131,7 +131,7 @@ inline constexpr void convert_type_fundamental_or_array(T_To& to,
                   is_signed_v<T_To_El> == is_signed_v<T_From_El>) {
       // Sanity check - this should definitely be true
       static_assert(sizeof(T_From_C) == sizeof(T_To_C));
-      std::memcpy(&to, &from, sizeof(T_To_C));
+      memcpy(&to, &from, sizeof(T_To_C));
     } else {
       for (size_t i = 0; i < std::extent_v<T_To_C>; i++) {
         convert_type_fundamental_or_array(to[i], from[i]);