Backed out changeset f74adc43b654 (bug 1608645) because it was made obsolete by bug 1610790.
authorRyan VanderMeulen <ryanvm@gmail.com>
Wed, 12 Feb 2020 10:39:49 -0500
changeset 575713 826c0406674581a02402050bff4b7099263e871f
parent 575712 c709cccd729086414058a39da1d7821a8938c6e9
child 575714 41c32b8d5671f497f640093a08a69eead4e8dbf1
push id12697
push userryanvm@gmail.com
push dateThu, 13 Feb 2020 19:44:45 +0000
treeherdermozilla-beta@df6323bac412 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1608645, 1610790
milestone74.0
backs outf74adc43b654c1e663d9e62be89effc5fb76d4bd
Backed out changeset f74adc43b654 (bug 1608645) because it was made obsolete by bug 1610790.
mozglue/misc/NativeNt.h
mozglue/misc/interceptor/MMPolicies.h
mozglue/tests/TestNativeNt.cpp
--- a/mozglue/misc/NativeNt.h
+++ b/mozglue/misc/NativeNt.h
@@ -526,64 +526,55 @@ class MOZ_RAII PEHeaders final {
   }
 
   /**
    * This functions searches the export table for a given string as
    * GetProcAddress does.  Instead of a function address, this returns a matched
    * entry of the Export Address Table i.e. a pointer to an RVA of a matched
    * function.  If the entry is forwarded, this function returns nullptr.
    */
-  LauncherResult<const DWORD*> FindExportAddressTableEntry(
-      const char* aFunctionNameASCII) {
+  const DWORD* FindExportAddressTableEntry(const char* aFunctionNameASCII) {
     struct NameTableComparator {
       NameTableComparator(PEHeaders& aPEHeader, const char* aTarget)
           : mPEHeader(aPEHeader), mTarget(aTarget) {}
 
       int operator()(DWORD aOther) const {
-        // Use RVAToPtrUnchecked because StrcmpASCII does not accept nullptr.
-        return StrcmpASCII(mTarget,
-                           mPEHeader.RVAToPtrUnchecked<const char*>(aOther));
+        return StrcmpASCII(mTarget, mPEHeader.RVAToPtr<const char*>(aOther));
       }
 
       PEHeaders& mPEHeader;
       const char* mTarget;
     };
 
     DWORD rvaDirStart, rvaDirEnd;
     const auto exportDir = GetImageDirectoryEntry<PIMAGE_EXPORT_DIRECTORY>(
         IMAGE_DIRECTORY_ENTRY_EXPORT, &rvaDirStart, &rvaDirEnd);
     if (!exportDir) {
-      return LAUNCHER_ERROR_FROM_WIN32(ERROR_BAD_EXE_FORMAT);
+      return nullptr;
     }
 
     const auto exportAddressTable =
         RVAToPtr<const DWORD*>(exportDir->AddressOfFunctions);
     const auto exportNameTable =
         RVAToPtr<const DWORD*>(exportDir->AddressOfNames);
     const auto exportOrdinalTable =
         RVAToPtr<const WORD*>(exportDir->AddressOfNameOrdinals);
 
-    // If any of these tables are modified and located outside the mapped image,
-    // we don't search and bail out.
-    if (!exportAddressTable || !exportNameTable || !exportOrdinalTable) {
-      return LAUNCHER_ERROR_FROM_WIN32(ERROR_BAD_EXE_FORMAT);
-    }
-
     const NameTableComparator comp(*this, aFunctionNameASCII);
 
     size_t match;
     if (!BinarySearchIf(exportNameTable, 0, exportDir->NumberOfNames, comp,
                         &match)) {
-      return LAUNCHER_ERROR_FROM_WIN32(ERROR_PROC_NOT_FOUND);
+      return nullptr;
     }
 
     WORD index = exportOrdinalTable[match];
 
     if (index >= exportDir->NumberOfFunctions) {
-      return LAUNCHER_ERROR_FROM_WIN32(ERROR_BAD_EXE_FORMAT);
+      return nullptr;
     }
 
     DWORD rvaFunction = exportAddressTable[index];
     if (rvaFunction >= rvaDirStart && rvaFunction < rvaDirEnd) {
       // If an entry points to an address within the export section, the
       // field is a forwarder RVA.  We return nullptr because the entry is
       // not a function address but a null-terminated string used for export
       // forwarding.
--- a/mozglue/misc/interceptor/MMPolicies.h
+++ b/mozglue/misc/interceptor/MMPolicies.h
@@ -666,43 +666,37 @@ class MMPolicyOutOfProcess : public MMPo
     MEMORY_BASIC_INFORMATION mbi;
     SIZE_T result = ::VirtualQueryEx(mProcess, aVAddress, &mbi, sizeof(mbi));
 
     return result && mbi.AllocationProtect && (mbi.Type & MEM_IMAGE) &&
            mbi.State == MEM_COMMIT && mbi.Protect != PAGE_NOACCESS;
   }
 
   /**
-   * This searches the current process's export address table for a given name,
-   * but retrieves an RVA from a corresponding table entry in the target process
-   * because the table entry in the current process might have been replaced.
+   * This searches the target process's export address table for a given name
+   * instead of simply calling ::GetProcAddress because the local export table
+   * might be modified and the value of a table entry might be different from
+   * the target process.  If we fail to get an entry for some reason, we fall
+   * back to using ::GetProcAddress.
    */
   FARPROC GetProcAddress(HMODULE aModule, const char* aName) const {
     nt::PEHeaders moduleHeaders(aModule);
-    auto funcEntry = moduleHeaders.FindExportAddressTableEntry(aName);
-    if (funcEntry.isErr()) {
-      // FindExportAddressTableEntry fails if |aName| is invalid or the entire
-      // export table has been modified.  In the former case, ::GetProcAddress
-      // will return nullptr.  In the latter case, funcEntry may point to an
-      // invalid address in the target process.  We bail out in both cases.
-      return nullptr;
-    }
-    if (!funcEntry.inspect()) {
+    const DWORD* funcEntry = moduleHeaders.FindExportAddressTableEntry(aName);
+    if (!funcEntry) {
       // FindExportAddressTableEntry returns nullptr if a matched entry is
       // forwarded to another module.  Because a forwarder entry needs to point
       // a null-terminated string within the export section, it's less likely to
       // be modified by a third-party code.  We safely use the local table.
       return ::GetProcAddress(aModule, aName);
     }
 
     SIZE_T numBytes = 0;
     DWORD rvaTargetFunction = 0;
-    BOOL ok =
-        ::ReadProcessMemory(mProcess, funcEntry.inspect(), &rvaTargetFunction,
-                            sizeof(rvaTargetFunction), &numBytes);
+    BOOL ok = ::ReadProcessMemory(mProcess, funcEntry, &rvaTargetFunction,
+                                  sizeof(rvaTargetFunction), &numBytes);
     if (!ok || numBytes != sizeof(rvaTargetFunction)) {
       // If we fail to read the table entry in the target process for unexpected
       // reason, we fall back to ::GetProcAddress.
       return ::GetProcAddress(aModule, aName);
     }
 
     return moduleHeaders.RVAToPtr<FARPROC>(rvaTargetFunction);
   }
--- a/mozglue/tests/TestNativeNt.cpp
+++ b/mozglue/tests/TestNativeNt.cpp
@@ -218,40 +218,37 @@ int main(int argc, char* argv[]) {
   // Use ntdll.dll because it does not have any forwarder RVA.
   HMODULE ntdllImageBase = ::GetModuleHandleW(L"ntdll.dll");
   PEHeaders ntdllHeaders(ntdllImageBase);
 
   auto exportDir = ntdllHeaders.GetExportDirectory();
   auto tableOfNames = ntdllHeaders.RVAToPtr<PDWORD>(exportDir->AddressOfNames);
   for (DWORD i = 0; i < exportDir->NumberOfNames; ++i) {
     const auto name = ntdllHeaders.RVAToPtr<const char*>(tableOfNames[i]);
-    auto funcEntry = ntdllHeaders.FindExportAddressTableEntry(name);
-    if (funcEntry.isErr() || !funcEntry.inspect() ||
-        ntdllHeaders.RVAToPtr<const void*>(*funcEntry.inspect()) !=
-            ::GetProcAddress(ntdllImageBase, name)) {
+    const DWORD* funcEntry = ntdllHeaders.FindExportAddressTableEntry(name);
+    if (ntdllHeaders.RVAToPtr<const void*>(*funcEntry) !=
+        ::GetProcAddress(ntdllImageBase, name)) {
       printf(
           "TEST-FAILED | NativeNt | FindExportAddressTableEntry returned "
           "a wrong value.\n");
       return 1;
     }
   }
 
   // Test a known forwarder RVA.
-  auto funcEntry = k32headers.FindExportAddressTableEntry("HeapAlloc");
-  if (funcEntry.isErr() || funcEntry.inspect()) {
+  if (k32headers.FindExportAddressTableEntry("HeapAlloc")) {
     printf(
         "TEST-FAILED | NativeNt | kernel32!HeapAlloc should be forwarded to "
         "ntdll!RtlAllocateHeap.\n");
     return 1;
   }
 
   // Test an invalid name.
-  funcEntry = k32headers.FindExportAddressTableEntry("Invalid name");
-  if (funcEntry.isOk()) {
+  if (k32headers.FindExportAddressTableEntry("Invalid name")) {
     printf(
         "TEST-FAILED | NativeNt | FindExportAddressTableEntry should return "
-        "an error for a non-existent name.\n");
+        "null for an non-existent name.\n");
     return 1;
   }
 
   printf("TEST-PASS | NativeNt | All tests ran successfully\n");
   return 0;
 }