Bug 1705579 - Skip WinIOAutoObservation if TLS is not available. r=gerald,aklotz
authorToshihito Kikuchi <tkikuchi@mozilla.com>
Wed, 05 May 2021 17:00:09 +0000
changeset 578620 92f69679fcb9b11382992569e8335e6b20c52881
parent 578619 71b431aa1fc1b24055ca38bbded0064bafaf3387
child 578621 ba92ff62f0965c433f2926fc6af7e11f7ed10dff
push id142603
push usertkikuchi@mozilla.com
push dateWed, 05 May 2021 17:02:34 +0000
treeherderautoland@92f69679fcb9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald, aklotz
bugs1705579
milestone90.0a1
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
Bug 1705579 - Skip WinIOAutoObservation if TLS is not available. r=gerald,aklotz We hook several file APIs to record I/O performance data. Since TLS is not allocated in ntdll's loader worker thread, however, if someone does a file operation, we hit read AV because `WinIOAutoObservation` uses `nsString` and a thread local variable. Currently we can see this crash happens only when a DLL rule of AppLocker is defined, but theoretically this can happen when any module loaded in a worker thread does file operation in its entrypoint. The proposed fix is to skip `WinIOAutoObservation` if TLS is not available. Differential Revision: https://phabricator.services.mozilla.com/D113032
xpcom/base/nsWindowsHelpers.h
xpcom/build/IOInterposer.cpp
xpcom/build/PoisonIOInterposerWin.cpp
xpcom/tests/windows/TestPoisonIOInterposer.cpp
xpcom/tests/windows/moz.build
--- a/xpcom/base/nsWindowsHelpers.h
+++ b/xpcom/base/nsWindowsHelpers.h
@@ -294,16 +294,20 @@ HMODULE inline LoadLibrarySystem32(LPCWS
 
 }  // namespace
 
 // for UniquePtr
 struct LocalFreeDeleter {
   void operator()(void* aPtr) { ::LocalFree(aPtr); }
 };
 
+struct VirtualFreeDeleter {
+  void operator()(void* aPtr) { ::VirtualFree(aPtr, 0, MEM_RELEASE); }
+};
+
 // for UniquePtr to store a PSID
 struct FreeSidDeleter {
   void operator()(void* aPtr) { ::FreeSid(aPtr); }
 };
 // Unfortunately, although SID is a struct, PSID is a void*
 // This typedef will work for storing a PSID in a UniquePtr and should make
 // things a bit more readable.
 typedef mozilla::UniquePtr<void, FreeSidDeleter> UniqueSidPtr;
--- a/xpcom/build/IOInterposer.cpp
+++ b/xpcom/build/IOInterposer.cpp
@@ -508,20 +508,20 @@ void IOInterposer::RegisterCurrentThread
   PerThreadData* curThreadData = new PerThreadData(aIsMainThread);
   sThreadLocalData.set(curThreadData);
 }
 
 void IOInterposer::UnregisterCurrentThread() {
   if (!sThreadLocalDataInitialized) {
     return;
   }
-  PerThreadData* curThreadData = sThreadLocalData.get();
-  MOZ_ASSERT(curThreadData);
-  sThreadLocalData.set(nullptr);
-  delete curThreadData;
+  if (PerThreadData* curThreadData = sThreadLocalData.get()) {
+    sThreadLocalData.set(nullptr);
+    delete curThreadData;
+  }
 }
 
 void IOInterposer::EnteringNextStage() {
   if (!sMasterList) {
     return;
   }
   NextStageObservation observation;
   Report(observation);
--- a/xpcom/build/PoisonIOInterposerWin.cpp
+++ b/xpcom/build/PoisonIOInterposerWin.cpp
@@ -14,16 +14,17 @@
 #include <windows.h>
 #include <winternl.h>
 
 #include "mozilla/Assertions.h"
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/FileUtilsWin.h"
 #include "mozilla/IOInterposer.h"
 #include "mozilla/Mutex.h"
+#include "mozilla/NativeNt.h"
 #include "mozilla/SmallArrayLRUCache.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/UniquePtr.h"
 #include "nsTArray.h"
 #include "nsWindowsDllInterceptor.h"
 #include "plstr.h"
 
 #ifdef MOZ_REPLACE_MALLOC
@@ -251,29 +252,36 @@ static mozilla::WindowsDllInterceptor::F
     gOriginalNtQueryFullAttributesFile;
 
 static NTSTATUS NTAPI InterposedNtCreateFile(
     PHANDLE aFileHandle, ACCESS_MASK aDesiredAccess,
     POBJECT_ATTRIBUTES aObjectAttributes, PIO_STATUS_BLOCK aIoStatusBlock,
     PLARGE_INTEGER aAllocationSize, ULONG aFileAttributes, ULONG aShareAccess,
     ULONG aCreateDisposition, ULONG aCreateOptions, PVOID aEaBuffer,
     ULONG aEaLength) {
+  // Something is badly wrong if this function is undefined
+  MOZ_ASSERT(gOriginalNtCreateFile);
+
+  if (!mozilla::nt::RtlGetThreadLocalStoragePointer()) {
+    return gOriginalNtCreateFile(
+        aFileHandle, aDesiredAccess, aObjectAttributes, aIoStatusBlock,
+        aAllocationSize, aFileAttributes, aShareAccess, aCreateDisposition,
+        aCreateOptions, aEaBuffer, aEaLength);
+  }
+
   // Report IO
   const wchar_t* buf =
       aObjectAttributes ? aObjectAttributes->ObjectName->Buffer : L"";
   uint32_t len = aObjectAttributes
                      ? aObjectAttributes->ObjectName->Length / sizeof(WCHAR)
                      : 0;
   nsDependentSubstring filename(buf, len);
   WinIOAutoObservation timer(mozilla::IOInterposeObserver::OpCreateOrOpen,
                              filename);
 
-  // Something is badly wrong if this function is undefined
-  MOZ_ASSERT(gOriginalNtCreateFile);
-
   // Execute original function
   NTSTATUS status = gOriginalNtCreateFile(
       aFileHandle, aDesiredAccess, aObjectAttributes, aIoStatusBlock,
       aAllocationSize, aFileAttributes, aShareAccess, aCreateDisposition,
       aCreateOptions, aEaBuffer, aEaLength);
   if (NT_SUCCESS(status) && aFileHandle) {
     timer.SetHandle(*aFileHandle);
   }
@@ -281,110 +289,141 @@ static NTSTATUS NTAPI InterposedNtCreate
 }
 
 static NTSTATUS NTAPI InterposedNtReadFile(HANDLE aFileHandle, HANDLE aEvent,
                                            PIO_APC_ROUTINE aApc, PVOID aApcCtx,
                                            PIO_STATUS_BLOCK aIoStatus,
                                            PVOID aBuffer, ULONG aLength,
                                            PLARGE_INTEGER aOffset,
                                            PULONG aKey) {
+  // Something is badly wrong if this function is undefined
+  MOZ_ASSERT(gOriginalNtReadFile);
+
+  if (!mozilla::nt::RtlGetThreadLocalStoragePointer()) {
+    return gOriginalNtReadFile(aFileHandle, aEvent, aApc, aApcCtx, aIoStatus,
+                               aBuffer, aLength, aOffset, aKey);
+  }
+
   // Report IO
   WinIOAutoObservation timer(mozilla::IOInterposeObserver::OpRead, aFileHandle,
                              aOffset);
 
-  // Something is badly wrong if this function is undefined
-  MOZ_ASSERT(gOriginalNtReadFile);
-
   // Execute original function
   return gOriginalNtReadFile(aFileHandle, aEvent, aApc, aApcCtx, aIoStatus,
                              aBuffer, aLength, aOffset, aKey);
 }
 
 static NTSTATUS NTAPI InterposedNtReadFileScatter(
     HANDLE aFileHandle, HANDLE aEvent, PIO_APC_ROUTINE aApc, PVOID aApcCtx,
     PIO_STATUS_BLOCK aIoStatus, FILE_SEGMENT_ELEMENT* aSegments, ULONG aLength,
     PLARGE_INTEGER aOffset, PULONG aKey) {
+  // Something is badly wrong if this function is undefined
+  MOZ_ASSERT(gOriginalNtReadFileScatter);
+
+  if (!mozilla::nt::RtlGetThreadLocalStoragePointer()) {
+    return gOriginalNtReadFileScatter(aFileHandle, aEvent, aApc, aApcCtx,
+                                      aIoStatus, aSegments, aLength, aOffset,
+                                      aKey);
+  }
+
   // Report IO
   WinIOAutoObservation timer(mozilla::IOInterposeObserver::OpRead, aFileHandle,
                              aOffset);
 
-  // Something is badly wrong if this function is undefined
-  MOZ_ASSERT(gOriginalNtReadFileScatter);
-
   // Execute original function
   return gOriginalNtReadFileScatter(aFileHandle, aEvent, aApc, aApcCtx,
                                     aIoStatus, aSegments, aLength, aOffset,
                                     aKey);
 }
 
 // Interposed NtWriteFile function
 static NTSTATUS NTAPI InterposedNtWriteFile(HANDLE aFileHandle, HANDLE aEvent,
                                             PIO_APC_ROUTINE aApc, PVOID aApcCtx,
                                             PIO_STATUS_BLOCK aIoStatus,
                                             PVOID aBuffer, ULONG aLength,
                                             PLARGE_INTEGER aOffset,
                                             PULONG aKey) {
+  // Something is badly wrong if this function is undefined
+  MOZ_ASSERT(gOriginalNtWriteFile);
+
+  if (!mozilla::nt::RtlGetThreadLocalStoragePointer()) {
+    return gOriginalNtWriteFile(aFileHandle, aEvent, aApc, aApcCtx, aIoStatus,
+                                aBuffer, aLength, aOffset, aKey);
+  }
+
   // Report IO
   WinIOAutoObservation timer(mozilla::IOInterposeObserver::OpWrite, aFileHandle,
                              aOffset);
 
-  // Something is badly wrong if this function is undefined
-  MOZ_ASSERT(gOriginalNtWriteFile);
-
   // Execute original function
   return gOriginalNtWriteFile(aFileHandle, aEvent, aApc, aApcCtx, aIoStatus,
                               aBuffer, aLength, aOffset, aKey);
 }
 
 // Interposed NtWriteFileGather function
 static NTSTATUS NTAPI InterposedNtWriteFileGather(
     HANDLE aFileHandle, HANDLE aEvent, PIO_APC_ROUTINE aApc, PVOID aApcCtx,
     PIO_STATUS_BLOCK aIoStatus, FILE_SEGMENT_ELEMENT* aSegments, ULONG aLength,
     PLARGE_INTEGER aOffset, PULONG aKey) {
+  // Something is badly wrong if this function is undefined
+  MOZ_ASSERT(gOriginalNtWriteFileGather);
+
+  if (!mozilla::nt::RtlGetThreadLocalStoragePointer()) {
+    return gOriginalNtWriteFileGather(aFileHandle, aEvent, aApc, aApcCtx,
+                                      aIoStatus, aSegments, aLength, aOffset,
+                                      aKey);
+  }
+
   // Report IO
   WinIOAutoObservation timer(mozilla::IOInterposeObserver::OpWrite, aFileHandle,
                              aOffset);
 
-  // Something is badly wrong if this function is undefined
-  MOZ_ASSERT(gOriginalNtWriteFileGather);
-
   // Execute original function
   return gOriginalNtWriteFileGather(aFileHandle, aEvent, aApc, aApcCtx,
                                     aIoStatus, aSegments, aLength, aOffset,
                                     aKey);
 }
 
 static NTSTATUS NTAPI InterposedNtFlushBuffersFile(
     HANDLE aFileHandle, PIO_STATUS_BLOCK aIoStatusBlock) {
+  // Something is badly wrong if this function is undefined
+  MOZ_ASSERT(gOriginalNtFlushBuffersFile);
+
+  if (!mozilla::nt::RtlGetThreadLocalStoragePointer()) {
+    return gOriginalNtFlushBuffersFile(aFileHandle, aIoStatusBlock);
+  }
+
   // Report IO
   WinIOAutoObservation timer(mozilla::IOInterposeObserver::OpFSync, aFileHandle,
                              nullptr);
 
-  // Something is badly wrong if this function is undefined
-  MOZ_ASSERT(gOriginalNtFlushBuffersFile);
-
   // Execute original function
   return gOriginalNtFlushBuffersFile(aFileHandle, aIoStatusBlock);
 }
 
 static NTSTATUS NTAPI InterposedNtQueryFullAttributesFile(
     POBJECT_ATTRIBUTES aObjectAttributes,
     PFILE_NETWORK_OPEN_INFORMATION aFileInformation) {
+  // Something is badly wrong if this function is undefined
+  MOZ_ASSERT(gOriginalNtQueryFullAttributesFile);
+
+  if (!mozilla::nt::RtlGetThreadLocalStoragePointer()) {
+    return gOriginalNtQueryFullAttributesFile(aObjectAttributes,
+                                              aFileInformation);
+  }
+
   // Report IO
   const wchar_t* buf =
       aObjectAttributes ? aObjectAttributes->ObjectName->Buffer : L"";
   uint32_t len = aObjectAttributes
                      ? aObjectAttributes->ObjectName->Length / sizeof(WCHAR)
                      : 0;
   nsDependentSubstring filename(buf, len);
   WinIOAutoObservation timer(mozilla::IOInterposeObserver::OpStat, filename);
 
-  // Something is badly wrong if this function is undefined
-  MOZ_ASSERT(gOriginalNtQueryFullAttributesFile);
-
   // Execute original function
   return gOriginalNtQueryFullAttributesFile(aObjectAttributes,
                                             aFileInformation);
 }
 
 }  // namespace
 
 /******************************** IO Poisoning ********************************/
new file mode 100644
--- /dev/null
+++ b/xpcom/tests/windows/TestPoisonIOInterposer.cpp
@@ -0,0 +1,152 @@
+/* -*- 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 <windows.h>
+
+#include "gtest/gtest.h"
+#include "mozilla/IOInterposer.h"
+#include "mozilla/NativeNt.h"
+#include "nsWindowsHelpers.h"
+
+using namespace mozilla;
+
+class TempFile final {
+  wchar_t mFullPath[MAX_PATH + 1];
+
+ public:
+  TempFile() : mFullPath{0} {
+    wchar_t tempDir[MAX_PATH + 1];
+    DWORD len = ::GetTempPathW(ArrayLength(tempDir), tempDir);
+    if (!len) {
+      return;
+    }
+
+    len = ::GetTempFileNameW(tempDir, L"poi", 0, mFullPath);
+    if (!len) {
+      return;
+    }
+  }
+
+  operator const wchar_t*() const { return mFullPath[0] ? mFullPath : nullptr; }
+};
+
+class Overlapped final {
+  nsAutoHandle mEvent;
+  OVERLAPPED mOverlapped;
+
+ public:
+  Overlapped()
+      : mEvent(::CreateEventW(nullptr, TRUE, FALSE, nullptr)), mOverlapped{} {
+    mOverlapped.hEvent = mEvent.get();
+  }
+
+  operator OVERLAPPED*() { return &mOverlapped; }
+
+  bool Wait(HANDLE aHandle) {
+    DWORD numBytes;
+    if (!::GetOverlappedResult(aHandle, &mOverlapped, &numBytes, TRUE)) {
+      return false;
+    }
+
+    return true;
+  }
+};
+
+const uint32_t kMagic = 0x12345678;
+
+void FileOpSync(const wchar_t* aPath) {
+  nsAutoHandle file(::CreateFileW(aPath, GENERIC_READ | GENERIC_WRITE,
+                                  FILE_SHARE_READ, nullptr, CREATE_ALWAYS,
+                                  FILE_ATTRIBUTE_NORMAL, nullptr));
+  ASSERT_NE(file.get(), INVALID_HANDLE_VALUE);
+
+  DWORD buffer = kMagic, numBytes = 0;
+  OVERLAPPED seek = {};
+  EXPECT_TRUE(WriteFile(file.get(), &buffer, sizeof(buffer), &numBytes, &seek));
+  EXPECT_TRUE(::FlushFileBuffers(file.get()));
+
+  seek.Offset = 0;
+  buffer = 0;
+  EXPECT_TRUE(
+      ::ReadFile(file.get(), &buffer, sizeof(buffer), &numBytes, &seek));
+  EXPECT_EQ(buffer, kMagic);
+
+  WIN32_FILE_ATTRIBUTE_DATA fullAttr = {};
+  EXPECT_TRUE(::GetFileAttributesExW(aPath, GetFileExInfoStandard, &fullAttr));
+}
+
+void FileOpAsync(const wchar_t* aPath) {
+  constexpr int kNumPages = 10;
+  constexpr int kPageSize = 4096;
+
+  Array<UniquePtr<void, VirtualFreeDeleter>, kNumPages> pages;
+  Array<FILE_SEGMENT_ELEMENT, kNumPages + 1> segments;
+  for (int i = 0; i < kNumPages; ++i) {
+    auto p = reinterpret_cast<uint32_t*>(::VirtualAlloc(
+        nullptr, kPageSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE));
+    ASSERT_TRUE(p);
+
+    pages[i].reset(p);
+    segments[i].Buffer = p;
+
+    p[0] = kMagic;
+  }
+
+  nsAutoHandle file(::CreateFileW(
+      aPath, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ, nullptr,
+      CREATE_ALWAYS,
+      FILE_ATTRIBUTE_NORMAL | FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED,
+      nullptr));
+  ASSERT_NE(file.get(), INVALID_HANDLE_VALUE);
+
+  Overlapped writeOp;
+  if (!::WriteFileGather(file.get(), segments.begin(), kNumPages * kPageSize,
+                         nullptr, writeOp)) {
+    EXPECT_EQ(::GetLastError(), ERROR_IO_PENDING);
+    EXPECT_TRUE(writeOp.Wait(file.get()));
+  }
+
+  for (int i = 0; i < kNumPages; ++i) {
+    *reinterpret_cast<uint32_t*>(pages[i].get()) = 0;
+  }
+
+  Overlapped readOp;
+  if (!::ReadFileScatter(file.get(), segments.begin(), kNumPages * kPageSize,
+                         nullptr, readOp)) {
+    EXPECT_EQ(::GetLastError(), ERROR_IO_PENDING);
+    EXPECT_TRUE(readOp.Wait(file.get()));
+  }
+
+  for (int i = 0; i < kNumPages; ++i) {
+    EXPECT_EQ(*reinterpret_cast<uint32_t*>(pages[i].get()), kMagic);
+  }
+}
+
+TEST(PoisonIOInterposer, NormalThread)
+{
+  mozilla::IOInterposerInit ioInterposerGuard;
+  TempFile tempFile;
+  FileOpSync(tempFile);
+  FileOpAsync(tempFile);
+  EXPECT_TRUE(::DeleteFileW(tempFile));
+}
+
+TEST(PoisonIOInterposer, NullTlsPointer)
+{
+  void* originalTls = mozilla::nt::RtlGetThreadLocalStoragePointer();
+  mozilla::IOInterposerInit ioInterposerGuard;
+
+  // Simulate a loader worker thread (TEB::LoaderWorker = 1)
+  // where ThreadLocalStorage is never allocated.
+  mozilla::nt::RtlSetThreadLocalStoragePointerForTestingOnly(nullptr);
+
+  TempFile tempFile;
+  FileOpSync(tempFile);
+  FileOpAsync(tempFile);
+  EXPECT_TRUE(::DeleteFileW(tempFile));
+
+  mozilla::nt::RtlSetThreadLocalStoragePointerForTestingOnly(originalTls);
+}
--- a/xpcom/tests/windows/moz.build
+++ b/xpcom/tests/windows/moz.build
@@ -2,15 +2,16 @@
 # vim: set filetype=python:
 # 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/.
 
 UNIFIED_SOURCES += [
     "TestCOM.cpp",
     "TestNtPathToDosPath.cpp",
+    "TestPoisonIOInterposer.cpp",
 ]
 
 OS_LIBS += [
     "mpr",
 ]
 
 FINAL_LIBRARY = "xul-gtest"