Bug 1230910 Remove some __try blocks and crash immediately draft
authorTom Ritter <tom@mozilla.com>
Wed, 08 Mar 2017 17:58:30 +0000
changeset 656085 b579918f14260303ffb48b66f13327b3cab2c47d
parent 656084 21b66ce499a9392a261ab1cb1874705f65b50589
child 656086 95bba1e797682e55f7a13e5139533dc9282b9142
push id77058
push userbmo:tom@mozilla.com
push dateWed, 30 Aug 2017 18:21:00 +0000
bugs1230910
milestone57.0a1
Bug 1230910 Remove some __try blocks and crash immediately MozReview-Commit-ID: 1xFpduDtjCf
security/sandbox/chromium/sandbox/win/src/crosscall_params.h
security/sandbox/chromium/sandbox/win/src/crosscall_server.cc
--- a/security/sandbox/chromium/sandbox/win/src/crosscall_params.h
+++ b/security/sandbox/chromium/sandbox/win/src/crosscall_params.h
@@ -235,22 +235,32 @@ class ActualCallParams : public CrossCal
       // It does not fit, abort copy.
       return false;
     }
 
     char* dest = reinterpret_cast<char*>(this) +  param_info_[index].offset_;
 
     // We might be touching user memory, this has to be done from inside a try
     // except.
-    __try {
+    /*
+     * This is a touch one, as this function is used in a lot (all?) IPC calls
+     * After spot-checking several of them, it seems that a failure here results
+     * in propogating an error up the stack and the operation simply fails and
+     * whatever expected it to succeed is just going to continue on and 
+     * maybe/probably cause an error later on.
+     *
+     * So commenting this out makes me nervous, and we'll probably just have 
+     * to do rigorous testing.
+     */
+      //__try {
       memcpy(dest, parameter_address, size);
-    }
-    __except(EXCEPTION_EXECUTE_HANDLER) {
-      return false;
-    }
+      //}
+      //__except(EXCEPTION_EXECUTE_HANDLER) {
+      //return false;
+      //}
 
     // Set the flag to tell the broker to update the buffer once the call is
     // made.
     if (is_in_out)
       SetIsInOut(true);
 
     param_info_[index + 1].offset_ = Align(param_info_[index].offset_ +
                                                 size);
--- a/security/sandbox/chromium/sandbox/win/src/crosscall_server.cc
+++ b/security/sandbox/chromium/sandbox/win/src/crosscall_server.cc
@@ -125,17 +125,26 @@ CrossCallParamsEx* CrossCallParamsEx::Cr
   char* backing_mem = NULL;
   uint32_t param_count = 0;
   uint32_t declared_size;
   uint32_t min_declared_size;
   CrossCallParamsEx* copied_params = NULL;
 
   // Touching the untrusted buffer is done under a SEH try block. This
   // will catch memory access violations so we don't crash.
-  __try {
+
+  /*
+   * An exception results in a 'false' being returned from InvokeCallback
+   * to indicate an error condition. This 'false' is ignored, and operations
+   * continue as if there was no error, and presumably cause an error at
+   * some other time. 
+   *
+   * Remove the __try/__except and crash immediately. Just have to test...
+   */
+  //__try {
     CrossCallParams* call_params =
         reinterpret_cast<CrossCallParams*>(buffer_base);
 
     // Check against the minimum size given the number of stated params
     // if too small we bail out.
     param_count = call_params->GetParamsCount();
     min_declared_size = sizeof(CrossCallParams) +
                         ((param_count + 1) * sizeof(ParamInfo));
@@ -163,22 +172,22 @@ CrossCallParamsEx* CrossCallParamsEx::Cr
     // Check that the copied buffer is still valid.
     if (copied_params->GetParamsCount() != param_count ||
         GetActualBufferSize(param_count, backing_mem) != declared_size ||
         !IsSizeWithinRange(buffer_size, min_declared_size, declared_size)) {
       delete [] backing_mem;
       return NULL;
     }
 
-  } __except(EXCEPTION_EXECUTE_HANDLER) {
+    //} __except(EXCEPTION_EXECUTE_HANDLER) {
     // In case of a windows exception we know it occurred while touching the
     // untrusted buffer so we bail out as is.
-    delete [] backing_mem;
-    return NULL;
-  }
+    //delete [] backing_mem;
+    //return NULL;
+    //}
 
   const char* last_byte = &backing_mem[declared_size];
   const char* first_byte = &backing_mem[min_declared_size];
 
   // Verify here that all and each parameters make sense. This is done in the
   // local copy.
   for (uint32_t ix = 0; ix != param_count; ++ix) {
     uint32_t size = 0;