Bug 1895375 - [1/4] Adjust types associated with ipc::LaunchError r=ipc-reviewers,win-reviewers,gstoll,nika

As the intended use for LaunchError::mFunction is telemetry, avoid the
possibility of accidental exfiltration of PII by requiring that
LaunchError be constructed from `StaticString`.

Additionally, remove the Windows-specific constructor overloads in favor
of an explicit factory function, and explicitly document that `mError`
is a generic bag of bits rather than any kind of strict error type.

No functional changes.

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

ipc/chromium/src/base/process_util_win.cc

@@ -262,7 +262,8 @@ Result<Ok, LaunchError> LaunchApp(const std::wstring& cmdline,
     if (SetHandleInformation(h, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT) ==
         0) {
       MOZ_DIAGNOSTIC_ASSERT(false, "SetHandleInformation failed");
-      return Err(LaunchError("SetHandleInformation", GetLastError()));
+      return Err(
+          LaunchError::FromWin32Error("SetHandleInformation", GetLastError()));
     }
     handlesToInherit.push_back(h);
   }
@@ -312,7 +313,7 @@ Result<Ok, LaunchError> LaunchApp(const std::wstring& cmdline,
   if (lpAttributeList) FreeThreadAttributeList(lpAttributeList);
   if (!createdOK) {
     DLOG(WARNING) << "CreateProcess Failed: " << GetLastError();
-    return Err(LaunchError("CreateProcess", GetLastError()));
+    return Err(LaunchError::FromWin32Error("CreateProcess", GetLastError()));
   }
 
   gProcessLog.print("==> process %d launched child process %d (%S)\n",

ipc/glue/GeckoChildProcessHost.cpp

@@ -831,9 +831,9 @@ bool GeckoChildProcessHost::AsyncLaunch(std::vector<std::string> aExtraOpts) {
 #if defined(XP_WIN)
                     "%s,0x%lx,%s",
 #else
-                    "%s,%d,%s",
+                    "%s,%ld,%s",
 #endif
-                    aError.FunctionName(), aError.ErrorCode(),
+                    aError.FunctionName().get(), aError.ErrorCode(),
                     XRE_GeckoProcessTypeToString(mProcessType));
                 // Max telemetry key is 72 chars
                 // https://searchfox.org/mozilla-central/rev/c244b16815d1fc827d141472b9faac5610f250e7/toolkit/components/telemetry/core/TelemetryScalar.cpp#105

ipc/glue/LaunchError.cpp

@@ -1,31 +0,0 @@
-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
-/* vim: set ts=8 sts=2 et sw=2 tw=80: */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-#include "LaunchError.h"
-
-#if defined(XP_WIN)
-#  include "mozilla/WinHeaderOnlyUtils.h"
-#endif
-
-namespace mozilla::ipc {
-
-LaunchError::LaunchError(const char* aFunction, OsError aError)
-    : mFunction(aFunction), mError(aError) {}
-
-#if defined(XP_WIN)
-LaunchError::LaunchError(const char* aFunction, PRErrorCode aError)
-    : mFunction(aFunction), mError((int)aError) {}
-
-LaunchError::LaunchError(const char* aFunction, DWORD aError)
-    : mFunction(aFunction),
-      mError(WindowsError::FromWin32Error(aError).AsHResult()) {}
-#endif  // defined(XP_WIN)
-
-const char* LaunchError::FunctionName() const { return mFunction; }
-
-OsError LaunchError::ErrorCode() const { return mError; }
-
-}  // namespace mozilla::ipc

ipc/glue/LaunchError.h

@@ -7,32 +7,62 @@
 #ifndef mozilla_ipc_LaunchError_h
 #define mozilla_ipc_LaunchError_h
 
-#include "prerror.h"
+#include "mozilla/StaticString.h"
+#include "nsError.h"  // for nsresult
 
 #if defined(XP_WIN)
 #  include <windows.h>
 #  include <winerror.h>
-using OsError = HRESULT;
-#else
-using OsError = int;
 #endif
 
 namespace mozilla::ipc {
 
 class LaunchError {
  public:
-  explicit LaunchError(const char* aFunction, OsError aError = 0);
-#if defined(XP_WIN)
-  explicit LaunchError(const char* aFunction, PRErrorCode aError);
-  explicit LaunchError(const char* aFunction, DWORD aError);
-#endif  // defined(XP_WIN)
+  // The context of an error code (at the very least, the function that emitted
+  // it and where it was called) is fundamentally necessary to interpret its
+  // value. Since we should have that, we're not too fussed about integral
+  // coercions, so long as we don't lose any bits.
+  //
+  // Under POSIX, the usual OS-error type is `int`. Under Windows, the usual
+  // error type is `HRESULT`, which (despite having bitfield semantics) is
+  // ultimately a typedef for a flavor of (signed) `long`. For simplicity, and
+  // in the absence of other constraints, our "error type" container is simply
+  // the join of these two.
+  using ErrorType = long;
 
-  const char* FunctionName() const;
-  OsError ErrorCode() const;
+  // Constructor.
+  //
+  // The default of `0` is intended for failure cases where we don't have an
+  // error code -- usually because the function we've called to determine
+  // failure has only a boolean return, but also for when we've detected some
+  // internal failure and there's no appropriate `nsresult` for it.
+  explicit LaunchError(StaticString aFunction, ErrorType aError = 0)
+      : mFunction(aFunction), mError(aError) {}
+
+#ifdef WIN32
+  // The error code returned by ::GetLastError() is (usually) not an HRESULT.
+  // This convenience-function maps its return values to the segment of
+  // HRESULT's namespace reserved for that.
+  //
+  // This is not formally necessary -- the error location should suffice to
+  // disambiguate -- but it's nice to have it converted when reading through
+  // error-values: `0x80070005` is marginally more human-meaningful than `5`.
+  static LaunchError FromWin32Error(StaticString aFunction, DWORD aError) {
+    return LaunchError(aFunction, HRESULT_FROM_WIN32(aError));
+  }
+#endif
+
+  // By design, `nsresult` does not implicitly coerce to an integer.
+  LaunchError(StaticString aFunction, nsresult aError)
+      : mFunction(aFunction), mError((ErrorType)aError) {}
+
+  StaticString FunctionName() const { return mFunction; }
+  ErrorType ErrorCode() const { return mError; }
 
  private:
-  const char* mFunction;
-  OsError mError;
+  StaticString mFunction;
+  ErrorType mError;
 };
 
 }  // namespace mozilla::ipc

ipc/glue/moz.build

@@ -183,7 +183,6 @@ UNIFIED_SOURCES += [
     "InputStreamUtils.cpp",
     "IPCMessageUtilsSpecializations.cpp",
     "IPCStreamUtils.cpp",
-    "LaunchError.cpp",
     "MessageChannel.cpp",
     "MessageLink.cpp",
     "MessagePump.cpp",

security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp

@@ -357,7 +357,8 @@ Result<Ok, mozilla::ipc::LaunchError> SandboxBroker::LaunchApp(
         "last_warning=%d",
         result, last_error, last_warning);
 
-    return Err(mozilla::ipc::LaunchError("SB::LA::SpawnTarget", last_error));
+    return Err(mozilla::ipc::LaunchError::FromWin32Error("SB::LA::SpawnTarget",
+                                                         last_error));
   } else if (sandbox::SBOX_ALL_OK != last_warning) {
     // If there was a warning (but the result was still ok), log it and proceed.
     LOG_W("Warning on SpawnTarget with last_error=%lu, last_warning=%d",