Bug 1836349 - [3/3] Restrict IPCResult not to accept nonconstant strings r=nika,ipc-reviewers

Text passed to `IPCResult::Fail` (and, therefore, text passed to
`IPC_FAIL`) may end up in telemetry.

To avoid accidentally capturing unwanted information, restrict all such
text to compile-time constant strings, unless approved by data-review.

Differential Revision: https://phabricator.services.mozilla.com/D180152

docshell/base/SyncedContextInlines.h

@@ -152,7 +152,8 @@ mozilla::ipc::IPCResult Transaction<Context>::CommitFromIPC(
     nsCString error = FormatValidationError<Context>(
         failedFields,
         "Invalid Transaction from Child - CanSet failed for field(s): ");
-    return IPC_FAIL(aSource, error.get());
+    // data-review+ at https://bugzilla.mozilla.org/show_bug.cgi?id=1618992#c7
+    return IPC_FAIL_UNSAFE_PRINTF(aSource, "%s", error.get());
   }
 
   // Validate may have dropped some fields from the transaction, check it's not

dom/ipc/ContentChild.cpp

@@ -1780,7 +1780,11 @@ mozilla::ipc::IPCResult ContentChild::RecvConstructBrowser(
       NS_WARNING(reason.get());
       return IPC_OK();
     }
-    return IPC_FAIL(this, reason.get());
+
+    // (these are the only possible values of `reason` at this point)
+    return browsingContext
+               ? IPC_FAIL(this, "discarded initial top BrowsingContext")
+               : IPC_FAIL(this, "missing initial top BrowsingContext");
   }
 
   if (xpc::IsInAutomation() &&

ipc/glue/MessageChannel.h

@@ -6,7 +6,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef ipc_glue_MessageChannel_h
-#define ipc_glue_MessageChannel_h 1
+#define ipc_glue_MessageChannel_h
 
 #include "ipc/EnumSerializer.h"
 #include "mozilla/Atomics.h"

ipc/glue/MessageLink.h

@@ -6,7 +6,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef ipc_glue_MessageLink_h
-#define ipc_glue_MessageLink_h 1
+#define ipc_glue_MessageLink_h
 
 #include <cstdint>
 #include "base/message_loop.h"

ipc/glue/ProtocolUtils.cpp

@@ -59,8 +59,9 @@ MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(
 
 namespace ipc {
 
-IPCResult IPCResult::Fail(NotNull<IProtocol*> actor, const char* where,
-                          const char* why) {
+/* static */
+IPCResult IPCResult::FailImpl(NotNull<IProtocol*> actor, const char* where,
+                              const char* why) {
   // Calls top-level protocol to handle the error.
   nsPrintfCString errorMsg("%s %s\n", where, why);
   actor->GetIPCChannel()->Listener()->ProcessingError(

ipc/glue/ProtocolUtils.h

@@ -5,7 +5,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_ipc_ProtocolUtils_h
-#define mozilla_ipc_ProtocolUtils_h 1
+#define mozilla_ipc_ProtocolUtils_h
 
 #include <cstddef>
 #include <cstdint>
@@ -28,6 +28,7 @@
 #include "mozilla/ipc/MessageLink.h"
 #include "mozilla/ipc/SharedMemory.h"
 #include "mozilla/ipc/Shmem.h"
+#include "nsPrintfCString.h"
 #include "nsTHashMap.h"
 #include "nsDebug.h"
 #include "nsISupports.h"
@@ -343,20 +344,61 @@ class IProtocol : public HasResultCodes {
 #define IPC_FAIL_NO_REASON(actor) \
   mozilla::ipc::IPCResult::Fail(WrapNotNull(actor), __func__)
 
+/*
+ * IPC_FAIL_UNSAFE_PRINTF(actor, format, ...)
+ *
+ * Create a failure IPCResult with a dynamic reason-string.
+ *
+ * @note This macro causes data collection because IPC failure reasons may be
+ * sent to crash-stats, where they are publicly visible. Firefox data stewards
+ * must do data review on usages of this macro.
+ */
+#define IPC_FAIL_UNSAFE_PRINTF(actor, format, ...) \
+  mozilla::ipc::IPCResult::FailUnsafePrintfImpl(   \
+      WrapNotNull(actor), __func__, nsPrintfCString(format, ##__VA_ARGS__))
+
 /**
- * All message deserializer and message handler should return this
- * type via above macros. We use a less generic name here to avoid
- * conflict with mozilla::Result because we have quite a few using
- * namespace mozilla::ipc; in the code base.
+ * All message deserializers and message handlers should return this type via
+ * the above macros. We use a less generic name here to avoid conflict with
+ * `mozilla::Result` because we have quite a few `using namespace mozilla::ipc;`
+ * in the code base.
+ *
+ * Note that merely constructing a failure-result, whether directly or via the
+ * IPC_FAIL macros, causes the associated error message to be processed
+ * immediately.
  */
 class IPCResult {
  public:
   static IPCResult Ok() { return IPCResult(true); }
-  static IPCResult Fail(NotNull<IProtocol*> aActor, const char* aWhere,
-                        const char* aWhy = "");
+
+  // IPC failure messages can sometimes end up in telemetry. As such, to avoid
+  // accidentally exfiltrating sensitive information without a data review, we
+  // require that they be constant strings.
+  template <size_t N, size_t M>
+  static IPCResult Fail(NotNull<IProtocol*> aActor, const char (&aWhere)[N],
+                        const char (&aWhy)[M]) {
+    return FailImpl(aActor, aWhere, aWhy);
+  }
+  template <size_t N>
+  static IPCResult Fail(NotNull<IProtocol*> aActor, const char (&aWhere)[N]) {
+    return FailImpl(aActor, aWhere, "");
+  }
+
   MOZ_IMPLICIT operator bool() const { return mSuccess; }
 
+  // Only used by IPC_FAIL_UNSAFE_PRINTF (q.v.). Do not call this directly. (Or
+  // at least get data-review's approval if you do.)
+  template <size_t N>
+  static IPCResult FailUnsafePrintfImpl(NotNull<IProtocol*> aActor,
+                                        const char (&aWhere)[N],
+                                        nsPrintfCString const& aWhy) {
+    return FailImpl(aActor, aWhere, aWhy.get());
+  }
+
  private:
+  static IPCResult FailImpl(NotNull<IProtocol*> aActor, const char* aWhere,
+                            const char* aWhy);
+
   explicit IPCResult(bool aResult) : mSuccess(aResult) {}
   bool mSuccess;
 };