Make sandbox allocation functions return an option type.

This forces the caller to check for allocation failure and not blindly
dereference the pointer and hit a null pointer.

Fixes #588

experiments/process_sandbox/include/process_sandbox/cxxsandbox.h

@@ -98,7 +98,12 @@ namespace sandbox
      */
     Ret operator()(Args... args)
     {
-      CallFrame* callframe = lib.alloc<CallFrame>();
+      auto cf = lib.alloc<CallFrame>();
+      if (!cf)
+      {
+        throw std::bad_alloc();
+      }
+      CallFrame* callframe = cf.value();
       callframe->args = std::forward_as_tuple(args...);
       lib.send(vtable_index, callframe);
       if constexpr (!std::is_void_v<Ret>)

experiments/process_sandbox/include/process_sandbox/sandbox.h

@@ -549,10 +549,12 @@ namespace sandbox
      * allocation failed.
      */
     void* alloc_in_sandbox(size_t bytes, size_t count);
+
     /**
      * Deallocate an allocation in the sandbox.
      */
     void dealloc_in_sandbox(void* ptr);
+
     /**
      * Start the child process.  On *NIX systems, this can be called within a
      * vfork context and so must not allocate or modify memory on the heap, or
@@ -620,12 +622,16 @@ namespace sandbox
      * would have its vtable incorrectly initialised.
      */
     template<typename T>
-    T* alloc(size_t count)
+    std::optional<T*> alloc(size_t count)
     {
       static_assert(
         std::is_standard_layout_v<T> && std::is_trivial_v<T>,
         "Arrays allocated in sandboxes must be POD types");
       T* array = static_cast<T*>(alloc_in_sandbox(sizeof(T), count));
+      if (array == nullptr)
+      {
+        return std::nullopt;
+      }
       for (size_t i = 0; i < count; i++)
       {
         new (&array[i]) T();
@@ -658,12 +664,16 @@ namespace sandbox
      * would have its vtable incorrectly initialised.
      */
     template<typename T, size_t Count>
-    T* alloc()
+    std::optional<T*> alloc()
     {
       static_assert(
         std::is_standard_layout_v<T> && std::is_trivial_v<T>,
         "Arrays allocated in sandboxes must be POD types");
       T* array = static_cast<T*>(alloc_in_sandbox(sizeof(T), Count));
+      if (array == nullptr)
+      {
+        return std::nullopt;
+      }
       for (size_t i = 0; i < Count; i++)
       {
         new (&array[i]) T();
@@ -678,14 +688,18 @@ namespace sandbox
      * with a vtable would have its vtable incorrectly initialised.
      */
     template<typename T, typename... Args>
-    T* alloc(Args&&... args)
+    std::optional<T*> alloc(Args&&... args)
     {
       static_assert(
         !std::is_polymorphic_v<T>,
         "Classes with vtables cannot be safely allocated in sandboxes from "
         "outside (nor can virtual functions be safely called).");
-      return new (alloc_in_sandbox(sizeof(T), 1))
-        T(std::forward<Args>(args)...);
+      void* ptr = alloc_in_sandbox(sizeof(T), 1);
+      if (ptr == nullptr)
+      {
+        return std::nullopt;
+      }
+      return new (ptr) T(std::forward<Args>(args)...);
     }
     /**
      * Free an object allocated in the sandbox.
@@ -703,9 +717,13 @@ namespace sandbox
     char* strdup(const char* str)
     {
       auto len = strlen(str);
-      char* ptr = alloc<char>(len);
-      memcpy(ptr, str, len);
-      return ptr;
+      std::optional<char*> ptr = alloc<char>(len);
+      if (!ptr)
+      {
+        return nullptr;
+      }
+      memcpy(*ptr, str, len);
+      return *ptr;
     }
 
     /**
@@ -790,10 +808,10 @@ namespace sandbox
      */
     friend class MemoryServiceProvider;
 
-	/**
-	 * CallbackDispatcher needs to be able to terminate a running sandbox.
-	 */
-	friend class CallbackDispatcher;
+    /**
+     * CallbackDispatcher needs to be able to terminate a running sandbox.
+     */
+    friend class CallbackDispatcher;
   };
 
   /**

experiments/process_sandbox/src/libsandbox.cc

@@ -535,7 +535,12 @@ namespace sandbox
           // Allocate space for everything.  No overflow checking here, but the
           // called code is trusted and we could only overflow if it returned a
           // nonsense `ai_addrlen`.
-          char* buffer = lib.alloc<char>((sizeof(addrinfo) * count) + extra);
+          auto b = lib.alloc<char>((sizeof(addrinfo) * count) + extra);
+          if (!b)
+          {
+            return return_int(EAI_MEMORY);
+          }
+          auto buffer = b.value();
           // The allocated space will be an array of `addrinfo`s, followed by
           // (variable-sized) `sockaddr`s.
           addrinfo* ais = reinterpret_cast<addrinfo*>(buffer);

experiments/process_sandbox/tests/sandbox-zlib.cc

@@ -29,9 +29,13 @@ struct UnsandboxedZlib
   struct
   {
     template<typename T>
-    T* alloc(size_t count = 1)
+    std::optional<T*> alloc(size_t count = 1)
     {
       T* array = static_cast<T*>(calloc(sizeof(T), count));
+      if (array == nullptr)
+      {
+        return std::nullopt;
+      }
       for (size_t i = 0; i < count; i++)
       {
         new (&array[i]) T();
@@ -69,14 +73,20 @@ void test(ZLib& sandbox, const char* file, std::vector<char>& result)
   SANDBOX_INVARIANT(fd >= 0, "Failed to open {}", file);
   static const size_t out_buffer_size = 1024;
   static const size_t in_buffer_size = 1024;
-  char* in = sandbox.lib.template alloc<char>(in_buffer_size);
-  char* out = sandbox.lib.template alloc<char>(out_buffer_size);
+  auto optional_in = sandbox.lib.template alloc<char>(in_buffer_size);
+  auto optional_out = sandbox.lib.template alloc<char>(out_buffer_size);
+  SANDBOX_INVARIANT(optional_in && optional_out, "Buffer allocation failed");
+  auto in = optional_in.value();
+  auto out = optional_out.value();
   // This is needed because deflateInit is a macro that implicitly passes
   // a string literal.
   char* version = sandbox.lib.strdup(ZLIB_VERSION);
 #undef ZLIB_VERSION
 #define ZLIB_VERSION version
-  z_stream* zs = sandbox.lib.template alloc<z_stream>();
+  auto ozs = sandbox.lib.template alloc<z_stream>();
+  SANDBOX_INVARIANT(ozs.has_value(), "Allocation failed");
+  z_stream* zs = ozs.value();
+
   memset(zs, 0, sizeof(*zs));
   zs->zalloc = Z_NULL;
   zs->zfree = Z_NULL;