Backed out changeset f74adc43b654 (bug 1608645) as requested by tkikuchi (toshi).
authorCosmin Sabou <csabou@mozilla.com>
Tue, 11 Feb 2020 23:19:56 +0200
changeset 513435 302a75886c589426687bdf1c8c530ef1c47d9006
parent 513434 eb423c8595139c36622ae9e42e1f62fb99120019
child 513436 b71aa86c8109f9e6b7b03fa7bc3fc255efb33174
push id107016
push usercsabou@mozilla.com
push dateTue, 11 Feb 2020 21:25:10 +0000
treeherderautoland@8bc8a16881df [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1608645
milestone75.0a1
backs outf74adc43b654c1e663d9e62be89effc5fb76d4bd
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset f74adc43b654 (bug 1608645) as requested by tkikuchi (toshi).
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;
 }