This is harder than it sounds: We need to be able to walk that stack without triggering a loader lock deadlock. We must be sure to test this while the Gecko profiler is running.

Anthony -- is this something for LLTT?

Oops! too aggressive triaging. :)

Is this the same as bug 1382943?

(In reply to David Major [:dmajor] from comment #3) > Is this the same as bug 1382943? This bug makes it an explicit goal that we want to be able to pull stacks when loading an unrecognized DLL, so I guess that bug should be a dependency.

Hey Carl, Based on our discussions, it sounds like we can reasonably separate this stuff into a few logical parts: 1) The part that obtains the call stack when a DLL is loaded; 2) The part that saves that stack somewhere when the DLL is unrecognized; 3) The part that retrieves that stack and adds it to the telemetry ping. == Some suggestions for part 2 == I should note that I have added an interface between mozglue and xul called DllServices. We currently use it to broadcast observer notifications whenever a new DLL is loaded (but this is not done synchronously because loader lock). See https://searchfox.org/mozilla-central/source/mozglue/build/WindowsDllServices.h and https://searchfox.org/mozilla-central/source/mozglue/build/WindowsDllBlocklist.cpp#938 It should be useful to you to use that interface to call out from mozglue into xul, or vice versa. An example of code that captures a call stack and then prepares it for telemetry can be found at: https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/HangMonitor.cpp#168 Once you're on the xul side, you can include mozilla/Telemetry.h and call mozilla::Telemetry::GetStackAndModules() to transform the stack walk into a space-efficient, telemetry-compatible format. Then you can save necessary info for later processing. Let's avoid saving duplicates if the same injected DLL is loaded more than once. == Suggestions for part 3 == The way I think we should do this is to modify the IDL for nsITelemetry to add a new read-only attribute called "injectedModules" or something similar. Use the declaration of the "chromeHangs" attribute in the same IDL file as a template for this. Then we can retrieve that property from the JS side of telemetry and add it to a ping. The implementation of that method will take the ProcessedStacks saved by part (2) and use the JSAPI to reflect them into JS. An example of that is: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#660 (Maybe we should just refactor the JSAPI bits so that we can share ProcessedStack reflection with ChromeHangs?) Have you used the JSAPI before? If you have, you may now stop reading :-) If not, it's not too bad once you grok some of the essentials. In particular, you'll need to read about GC rooting: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/GC_Rooting_Guide

Would you mind posting a WIP patch when you get a chance?

Attached a rough WIP. It will currently output verbose module load info via OutputDebugString(), and bubble up into the telemetry payload. Functionally what remains is to try and determine the trustworthiness of the DLLs as they're loaded. The main mechanism is in the new file DllGatekeeper.cpp. Determination of trust level is in DllGatekeeper.cpp!GetModuleTrustLevel. Currently it's marking almost everything as untrusted, just for my local dev purposes. I recognize that I'll need to change many things to use MFBT et al objects. Plenty of care is taken to ensure we don't mess too much with the performance of the loader. We only process events at the top of the stack to avoid being in the loader lock. If processing is happening already in some other thread, we have to abandon it without waiting. So there will be cases where we are going to skip DLL load events because it would be too complex to handle them gracefully. Another example of compromise is if a DLL load happens within the stack walking worker thread, which would cause MozStackWalkThread to re-enter and hang. We discussed moving the worker thread to xul, which I am working on.

Adding a new WIP patch. The key difference is that I'm making a crude attempt at evaluating the trust of DLLs. The results of this are logged as c:\temp\ffmodules.txt in tab-separated format. I used this locally to paste into a spreadsheet: https://docs.google.com/spreadsheets/d/1ro6KdQXzk0sdj6HOGNfcuB4rY4hk7zANsi2ODoiISB8/edit?usp=sharing WinDllServices.cpp::GetModuleTrustLevel applies a few rules which accumulate a trust score. If the score is >=100, we don't report it. I log these rules into the 1st column of the output file. The rules I have so far are: 1. If it's signed by MS or Mozilla, auto-pass. We really can't expect better validation than this. 2. If the version info is "Microsoft Corporation", +50 pts. 3. If it's in the same directory as the process EXE, +50 pts. -> 4. If it ALSO has the same version info as the process EXE, +50 pts. This accounts for both release & local dev builds of Firefox. 5. If it's in the same directory as kernel32.dll, +50 pts. -> 6. If the leaf name begins with "kbd" and has no version info, another +50 pts. This accounts for keyboard layout DLLs. 7. If it doesn't exist on disk, but is called "JitPI.dll", +100 pts. This is apparently the JIT profiler. On my system, this covers everything except an NVidia DLL called nvinitx.dll, which lives in c:\windows\system32\driverstore\filerepository\nvdm.inf_amd64_a74a38776cc5071e\ I'd love to see the output of this info on other systems before putting it to any public use.

Attaching new WIP.

Related try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c634ab3995debef3e2669924bd02cfa29158d6b0 The ccov oranges are false alarms I have been told. I'm using bit flags for the DLL eval results instead of the original string of characters; this feels more structured. The feature is currently restricted to the main process, and only on Nightly. I want to look at the I have some questions about organizing the code; as you can see there are some utility classes that are duplicated, Windows API wrappers that might need to move somewhere else, that kind of thing. I will also need help interpreting Talos results: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=c634ab3995debef3e2669924bd02cfa29158d6b0&framework=1&selectedTimeRange=172800

New patch for feedback, now using CaptureStackBackTrace() to resolve performance issues with the previous patch. Aaron, the try run you already saw ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=b469818fd5581e3a95a104a41b9a9eaf847de80e ) was using CaptureStackBackTrace() only for x86. For x64 I continued to use StackWalk64. But this patch uses CaptureStackBackTrace() for all architectures, in hopes of resolving that remaining x64 perf hit. Try run for this patch here (currently running as I write this): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1398c2871c8fcab98843a6754da50218e2f8fac0 Perfherder results: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=1398c2871c8fcab98843a6754da50218e2f8fac0

Comment on attachment 8988681 [details] [diff] [review] 1435827_20180629.txt Review of attachment 8988681 [details] [diff] [review]: ----------------------------------------------------------------- I can't see the entire code for UntrustedDllsProcessor.cpp in this patch; it appears to only contain a diff from the previous one. Also for some reason there are two UntrustedDllsProcessor.cpp diffs in this patch, and Splinter is very confused about how to display them. I'm going to need to see that file in its entirety. I've commented on everything else, but I had to punt on a few things due to lack of context. Re: splitting this up into multiple reviews, I'd suggest: 1) mozglue bits 2) WinDllServices.(h|cpp) 3) Telemetry bits. The final review for Telemetry code should go to a telemetry peer. I can review the other two patches. I'd also suggest kicking off the data review process concurrently with reviewing the patches. I don't expect the data review to require the final patches to be r+'d; just as long as they know the telemetry format and its contents, we should be good. General comment: Please review the coding style guide and the internal string guide and clean up the patches accordingly. ::: mozglue/build/MozglueUtils.h @@ +86,5 @@ > + bool mDidAcquire; > + SRWLOCK& mLock; > +}; > + > +struct RecursiveLock { Let's use class here instead of struct. This also needs style fixups. @@ +115,5 @@ > + > + autoRelease(autoRelease&& rhs) = default; > + > + explicit operator bool() { return mIsLocked; } > + bool operator !() { return !mIsLocked; } You don't need operator! since you've already implemented explicit operator bool ::: mozglue/build/UntrustedDllsProcessor.h @@ +11,5 @@ > +namespace mozilla { > +namespace glue { > + > +namespace detail { > + class DllServicesBase; Style Nit: you don't need to indent inside namespace blocks @@ +14,5 @@ > +namespace detail { > + class DllServicesBase; > +} // namespace detail > + > +struct UntrustedDllsProcessor { Nit: Style. Opening braces should be on a new line for class/struct/function ::: mozglue/build/WindowsDllBlocklist.cpp @@ +388,5 @@ > static NTSTATUS NTAPI > patched_LdrLoadDll (PWCHAR filePath, PULONG flags, PUNICODE_STRING moduleFileName, PHANDLE handle) > { > +#ifdef ENABLE_UNTRUSTED_DLL_PROCESSOR > + if (!(sInitFlags & eDllBlocklistInitFlagIsChildProcess)) { Since we're reusing this condition all over the place in this function, it might be more readable to just assign it to a bool at the top of the function. @@ +598,5 @@ > + if (handle) { > + *handle = myHandle; > + } > + } > + Nit: There are spaces in this blank line. Please remove. @@ +928,5 @@ > > gDllServices = aSvc; > + > +#ifdef ENABLE_UNTRUSTED_DLL_PROCESSOR > + if (!(sInitFlags & eDllBlocklistInitFlagIsChildProcess)) { Please combine these two predicates for && so we can drop the second if ::: mozglue/build/WindowsDllServices.h @@ +43,5 @@ > +MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(ModuleTrustFlags); > + > +// A C-friendly interface to ModuleLoadEvent > +struct MLE_ModuleInfo_C { > + uintptr_t base; Nit: field names should always be prefixed with 'm', eg mBase, mLdrName, etc. @@ +54,5 @@ > + uint64_t processUptimeMS; > + uintptr_t* stack; > + size_t stackSize; > + MLE_ModuleInfo_C* modules; > + size_t modulesSize; Can you rename this to clarify what modulesSize references? I'm assuming it's an array length? @@ +75,5 @@ > + // is guaranteed to be called in this case. > + virtual bool BeginNotifyUML(size_t aEventCount, > + int aMissedEvents, > + int aHitEvents) = 0; > + virtual void NotifyUML(const ModuleLoadEvent_C* event) = 0; s/event/aEvent/ @@ +167,5 @@ > virtual void DispatchDllLoadNotification(PCUNICODE_STRING aDllName) override {} > + virtual bool BeginNotifyUML(size_t aEventCount, > + int aMissedEvents, > + int aHitEvents) override { return false; } > + virtual void NotifyUML(const ModuleLoadEvent_C* event) override { } Nit: no space needed between {} ::: toolkit/xre/WinDllServices.cpp @@ +7,5 @@ > #include "mozilla/WinDllServices.h" > > +#include <process.h> > +#include <winternl.h> > +#include "mozilla/Atomics.h" Please add a blank line between the standard includes and the mozilla includes. @@ +44,5 @@ > + } > + } > +} > + > +struct RecursiveLock { I'd rather we not have two copies of this in the tree. @@ +118,5 @@ > +}; > + > +static RecursiveLock* sUntrustedModuleLoadEventsLock; > + > +struct VersionInfo { Style. Also: Please make these classes @@ +132,5 @@ > + VersionNumber(DWORD ms, DWORD ls) : > + version64((uint64_t)ms << 32 | ls) > + { } > + > + int a() const { return (version64 & 0xffff000000000000) >> 48; } This should return uint16_t, I think. @@ +150,5 @@ > + }; > + > + VersionNumber fileVersion; > + VersionNumber productVersion; > + Blank spaces on this line @@ +199,5 @@ > + } > +}; > + > +static void > +SplitPath(const nsString& fullPath, nsString& directory, nsString& leaf) Please use NS_NewLocalFile and nsIFile instead. Call GetLeafName and GetParent to obtain the leaf and directory, respectively. @@ +222,5 @@ > + size_t len; > + while (true) { > + //ret. retSize > + wchar_t *pwsz; > + size_t allocSize = ret.GetMutableData(&pwsz, bufferSize); Instead of using ret.GetMutableData, just call ret.SetLength to specify the correct length (including null), the call ret.BeginWriting to pass a pointer to GetModuleFileNameW. Then truncate the null terminator. For example, see https://searchfox.org/mozilla-central/source/accessible/windows/msaa/Compatibility.cpp#314 @@ +234,5 @@ > + } > + if (len == bufferSize && ::GetLastError() == ERROR_INSUFFICIENT_BUFFER) { > + bufferSize *= 2; > + } > + else { Style nit: } else { @@ +242,5 @@ > + } > + return ret; > +} > + > +typedef NTSTATUS(NTAPI *NtOpenDirectoryObject_T)( Now that we have bug 1473175, let's punt on KnownDLLs for the time being. Keep the code around for a potential follow-up, but let's remove it from the initial patch that we're planing to land. @@ +396,5 @@ > + 0, KEY_ENUMERATE_SUB_KEYS, > + &hk) != ERROR_SUCCESS) { > + return; > + } > + auto releasehk = MakeScopeExit([&]() { Let's use nsAutoRegKey here instead. @@ +401,5 @@ > + RegCloseKey(hk); > + }); > + > + DWORD iKey = 0; > + wchar_t strTemp[200]; Magic number @@ +403,5 @@ > + > + DWORD iKey = 0; > + wchar_t strTemp[200]; > + while (true) { > + DWORD strTempSize = ARRAYSIZE(strTemp); s/ARRAYSIZE/mozilla::ArrayLength/ @@ +414,5 @@ > + strTempSize = sizeof(strTemp); > + if (::RegGetValueW(hk, strTemp, L"Layout File", > + RRF_RT_REG_SZ, 0, strTemp, > + &strTempSize) == ERROR_SUCCESS) { > + nsString ws(strTemp); This should be a nsAutoString. Also pass in the number of chars derived from strTempSize so that you can avoid a redundant wcslen. @@ +429,5 @@ > + > +// Failures should return true. > +// This also fills in some more fields in ModuleInfo > +bool > +DllServices::IsModuleTrusted(ModuleLoadEvent_XUL::ModuleInfo& m, const ModuleLoadEvent_XUL& e) More descriptive parameter names, please. @@ +436,5 @@ > + // One with long path, one with short path. > + m.trustFlags = mozilla::glue::ModuleTrustFlags::eNone; > + > + if (m.fullPath.FindChar('~') != kNotFound) { > + nsString longPath; If you're instantiating strings on the stack, always use nsAutoString. ::: toolkit/xre/WinDllServices.h @@ +13,5 @@ > > namespace mozilla { > > +// XUL-friendly ModuleLoadEvent > +struct ModuleLoadEvent_XUL { Style nits @@ +24,5 @@ > + nsString leafName; > + nsCString fileVersion; > + }; > + > + void assign(const ModuleLoadEvent_XUL& rhs, bool alsoCopyModules = true); Function names should start with Uppercase letter. SpiderMonkey doesn't do this, but they use their own style. ::: toolkit/components/telemetry/Telemetry.cpp @@ +114,5 @@ > using mozilla::Telemetry::KeyedStackCapturer; > #endif > > +#if defined(XP_WIN) > +const int32_t kUntrustedModuleLoadEventsTelemetryVersion = 1; Also make this static @@ +132,5 @@ > sTelemetryIOObserver); > sTelemetryIOObserver = nullptr; > } > > +// Helper to convert mozilla::Vector<T> to a JS array object. We have other utility functions like this in TelemetryCommon.h, so I'd suggest moving this over there.

Comment on attachment 8993646 [details] Bug 1435827 part 3/3: Add telemetry for untrusted module loads; https://reviewboard.mozilla.org/r/258328/#review266032 ::: toolkit/components/telemetry/Telemetry.cpp:734 (Diff revision 1) > + > + > +NS_IMETHODIMP > +TelemetryImpl::GetUntrustedModuleLoadEvents(JSContext *cx, > + JS::MutableHandle<JS::Value> aRet) > +{ We're trying to shrink Telemetry.cpp to become more of a plumbing module. Can you move this implementation to a separate source file? That helps with the shrinking and a bit with ownership clarity. (In the future i want to move the various stack diagnostics to different components, but that's not important right now.) ::: toolkit/components/telemetry/TelemetrySession.jsm:1117 (Diff revision 1) > + payloadObj.untrustedModuleLoadEvents = protect(() => > + Telemetry.untrustedModuleLoadEvents); Will this be recorded for all users & for all Firefox channels, including release? If it's for all of release, do you have an estimate of the size impact (including upper bounds)?

Comment on attachment 8993641 [details] Request for data collection review form - Bug 1435827.txt > Please see document summarizing the ongoing discussion of this feature @ > https://docs.google.com/document/d/1-RwFbX6sNQOSU9JhKeNn9oRK17pMReHqrxHypcpyFiI/edit?usp=sharing That document is not publicly available. Can it be made public? If not, it would have to be removed from the review form since our data reviews need to be public. Is it possible that the stack traces can include user data? One thing that comes to mind in terms of leaking personal information is that the paths could include the username of the logged in user (e.g. C:\Users\<your user name>\...). Are you doing any filtering there to try and remove user details? Could we whitelist the directories we care about and send a code for each of them (e.g. "system32", "firefox", ...)?

Comment on attachment 8993644 [details] Bug 1435827 part 1/3: Send DLL load event info to xul; https://reviewboard.mozilla.org/r/258324/#review267924 I'm probably going to need another sitting to finish this, but I wanted to get you some feedback. I'm marking r- because we're going to need at least one more round. I'll probably add more to this review, so I'd suggest ensuring that any open issues are resolved before resubmitting. ::: mozglue/build/MozglueUtils.h:1 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public Please also add the two mode lines for emacs and vim above the license block for all new source files. ::: mozglue/build/MozglueUtils.h:5 (Diff revision 1) > +/* 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/. */ > + > +#ifndef mozilla_glue_Mozglueutils_h Nit: s/Mozglueutils/MozglueUtils (capitalization to match the name of the file) ::: mozglue/build/MozglueUtils.h:13 (Diff revision 1) > +#include <windows.h> > + > +namespace mozilla { > +namespace glue { > + > +class MOZ_RAII AutoSharedLock final add an include for mozilla/Attributes.h for MOZ_RAII ::: mozglue/build/MozglueUtils.h:65 (Diff revision 1) > +{ > +public: > + explicit AutoTryExclusiveLock(SRWLOCK& aLock) > + : mLock(aLock) > + { > + mDidAcquire = ::TryAcquireSRWLockExclusive(&aLock) != 0; Generally to coerce (BOOLEAN or BOOL) to bool, we use a double bang, e.g. ```c++ mDidAcquire = !!::TryAcquireSRWLockExclusive(&aLock); ``` ::: mozglue/build/UntrustedDllsProcessor.cpp:31 (Diff revision 1) > +namespace mozilla { > + > +extern Authenticode* GetAuthenticode(); > + > +void > +MozStackWalkFast(MozWalkStackCallback aCallback, uint32_t aSkipFrames, Since this code is the only consumer, let's not bother with MozStackWalkCallback and definte our own callback type to use here, since you're not using that third parameter anyway. ::: mozglue/build/UntrustedDllsProcessor.cpp:47 (Diff revision 1) > + > + if (sLimitFrameCount.isNothing()) { > + // According to MSDN, on Windows XP and Windows Server 2003: > + // "The sum of the FramesToSkip and FramesToCapture parameters must be less than 63" > + // Checking for OS major version >= 6 satisfies this check. > + OSVERSIONINFOEXW osvi = { 0 }; Our minimum system requirement is Windows 7, so please remove this check. ::: mozglue/build/UntrustedDllsProcessor.cpp:78 (Diff revision 1) > + > +static mozilla::UniquePtr<wchar_t[]> > +MakeString(const wchar_t* aOther) > +{ > + if (!aOther) { > + auto ret = mozilla::MakeUnique<wchar_t[]>(1); Can you just return nullptr here instead? ::: mozglue/build/UntrustedDllsProcessor.cpp:83 (Diff revision 1) > + auto ret = mozilla::MakeUnique<wchar_t[]>(1); > + ret[0] = 0; > + return ret; > + } > + auto ret = mozilla::MakeUnique<wchar_t[]>(wcslen(aOther) + 1); > + wcscpy(ret.get(), aOther); Use `wcscpy_s` ::: mozglue/build/UntrustedDllsProcessor.cpp:90 (Diff revision 1) > +} > + > +static mozilla::UniquePtr<wchar_t[]> > +MakeString(PUNICODE_STRING aOther) > +{ > + size_t chars = aOther->Length / 2; s/2/sizeof(wchar_t) ::: mozglue/build/UntrustedDllsProcessor.cpp:95 (Diff revision 1) > + size_t chars = aOther->Length / 2; > + if (chars > 500) { > + chars = 500; // Truncate for sanity > + } > + auto ret = mozilla::MakeUnique<wchar_t[]>(chars + 1); > + wcsncpy(ret.get(), aOther->Buffer, chars); let's use `wcscpy_s` here since that is much saner re: null termination. Then you don't need the following line anymore. ::: mozglue/build/UntrustedDllsProcessor.cpp:113 (Diff revision 1) > + mBase = aOther.mBase; > + mLdrName = MakeString(aOther.mLdrName.get()); > + mFullPath = MakeString(aOther.mFullPath.get()); > + } > + > + inline ModuleInfo() = default; Remove these `inline`s please. Since these are all in the class definiton, they are implicitly inline. ::: mozglue/build/UntrustedDllsProcessor.cpp:119 (Diff revision 1) > + inline virtual ~ModuleInfo() = default; > + inline ModuleInfo(const ModuleInfo& aOther) = delete; > + inline ModuleInfo operator =(const ModuleInfo& aOther) = delete; > + inline ModuleInfo operator =(ModuleInfo&& aOther) = delete; > + inline ModuleInfo(ModuleInfo&& aOther) > + { Since this function has move semantics, it should be moving, not copying. But `Assign` is copying. ::: mozglue/build/UntrustedDllsProcessor.cpp:124 (Diff revision 1) > + { > + Assign(aOther); > + } > + > + uintptr_t mBase; > + mozilla::UniquePtr<wchar_t[]> mLdrName; Would it make more sense to use `std::shared_ptr` and `std::make_shared` for these? We generally don't use those (most of our refcounting is intrusive), but I think it might make sense in this case. ::: mozglue/build/UntrustedDllsProcessor.cpp:128 (Diff revision 1) > + uintptr_t mBase; > + mozilla::UniquePtr<wchar_t[]> mLdrName; > + mozilla::UniquePtr<wchar_t[]> mFullPath; > + }; > + > + inline ModuleLoadEvent() = default; Same here ::: mozglue/build/UntrustedDllsProcessor.cpp:143 (Diff revision 1) > + inline void Assign(const ModuleLoadEvent& aOther) > + { > + for (auto& i : aOther.mModules) { > + ModuleInfo mcopy; > + mcopy.Assign(i); > + if (!mModules.append(std::move(mcopy))) { Use `Unused` here as I outlined elsewhere. ::: mozglue/build/UntrustedDllsProcessor.cpp:151 (Diff revision 1) > + } > + mIsStartup = aOther.mIsStartup; > + mThreadID = aOther.mThreadID; > + mProcessUptimeMS = aOther.mProcessUptimeMS; > + for (auto& i : aOther.mStack) { > + if (!mStack.append(i)) { `Unused` ::: mozglue/build/UntrustedDllsProcessor.cpp:173 (Diff revision 1) > + Vector<ModuleLoadEvent::ModuleInfo>& aStartupModules); > + > + Vector<ModuleLoadEvent> mModuleLoadEvents; > + Vector<mozilla::UniquePtr<wchar_t[]>> mModuleHistory; > + > + void _OnAfterModuleLoad(Vector<ModuleLoadEvent::ModuleInfo>& aModulesLoaded); No leading underscores please ::: mozglue/build/UntrustedDllsProcessor.cpp:185 (Diff revision 1) > + size_t allocated = MAX_PATH; > + mozilla::UniquePtr<wchar_t[]> ret = mozilla::MakeUnique<wchar_t[]>(allocated); > + size_t len; > + while (true) { > + len = (size_t)::GetModuleFileNameW((HMODULE)aModule, ret.get(), allocated); > + if (len == 0) { Generally we use operator! instead of checking for 0. eg ```c++ if (!len) { ``` ::: mozglue/build/UntrustedDllsProcessor.cpp:186 (Diff revision 1) > + mozilla::UniquePtr<wchar_t[]> ret = mozilla::MakeUnique<wchar_t[]>(allocated); > + size_t len; > + while (true) { > + len = (size_t)::GetModuleFileNameW((HMODULE)aModule, ret.get(), allocated); > + if (len == 0) { > + ret[0] = 0; Can you just return nullptr here? ::: mozglue/build/UntrustedDllsProcessor.cpp:192 (Diff revision 1) > + return ret; > + } > + if (len == allocated && ::GetLastError() == ERROR_INSUFFICIENT_BUFFER) { > + allocated *= 2; > + ret = mozilla::MakeUnique<wchar_t[]>(allocated); > + } `else` should be on the same line as `}`: ```c++ } else { ``` ::: mozglue/build/UntrustedDllsProcessor.cpp:197 (Diff revision 1) > + } > + else { > + break; > + } > + } > + for (size_t i = 0; i < len; ++ i) { no space between ++ and variable ::: mozglue/build/UntrustedDllsProcessor.cpp:219 (Diff revision 1) > + PLIST_ENTRY begin = &peb->Ldr->InMemoryOrderModuleList; > + PLIST_ENTRY flink = begin->Flink; > + PLDR_DATA_TABLE_ENTRY entry = 0; > + > + while (begin != flink) > + { `{` on the same line as `while` ::: mozglue/build/UntrustedDllsProcessor.cpp:221 (Diff revision 1) > + PLDR_DATA_TABLE_ENTRY entry = 0; > + > + while (begin != flink) > + { > + entry = CONTAINING_RECORD(flink, LDR_DATA_TABLE_ENTRY, InMemoryOrderLinks); > + if (entry) `{` on same line as `if` ::: mozglue/build/UntrustedDllsProcessor.cpp:228 (Diff revision 1) > + // Don't add to history; we can maybe catch better information live > + ModuleLoadEvent::ModuleInfo moduleInfo; > + moduleInfo.mBase = (uintptr_t)(entry->DllBase); > + moduleInfo.mLdrName = MakeString(&entry->FullDllName); > + moduleInfo.mFullPath = MakeString(&entry->FullDllName); > + if (!ret.emplaceBack(std::move(moduleInfo))) { Instead of doing this, include mozilla/Unused.h and then you can do this: ```c++ Unused << ret.emplaceBack(std::move(moduleInfo)); ``` ::: mozglue/build/UntrustedDllsProcessor.cpp:238 (Diff revision 1) > + flink = flink->Flink; > + } > + return ret; > +} > + > +static UntrustedDllsProcessorImpl* gUntrustedDllProcessorInstance = nullptr; Can you use `StaticAutoPtr` here? ::: mozglue/build/UntrustedDllsProcessor.cpp:253 (Diff revision 1) > +}; > + > +TLSData& > +UntrustedDllsProcessor_GetTLSData() > +{ > + thread_local TLSData sData; Please include mozilla/ThreadLocal.h and use MOZ_THREAD_LOCAL(TlsData) instead. Also please move this out of function scope. ::: mozglue/build/UntrustedDllsProcessor.cpp:297 (Diff revision 1) > +#endif // DEBUG > + > +void > +UntrustedDllsProcessor::EnterLoaderCall() > +{ > + UntrustedDllsProcessor_GetTLSData().mCallDepth ++; Nit: no space before ++ operator ::: mozglue/build/UntrustedDllsProcessor.cpp:303 (Diff revision 1) > +} > + > +void > +UntrustedDllsProcessor::ExitLoaderCall() > +{ > + UntrustedDllsProcessor_GetTLSData().mCallDepth --; Space before `--` ::: mozglue/build/UntrustedDllsProcessor.cpp:369 (Diff revision 1) > +} > + > +UntrustedDllsProcessorImpl::UntrustedDllsProcessorImpl(detail::DllServicesBase* aSvc, > + Vector<ModuleLoadEvent::ModuleInfo>& aStartupModules) > +{ > + sMissedMLE = 0; These will be 0 by default, so you don't need to assign the initial values ::: mozglue/build/UntrustedDllsProcessor.cpp:401 (Diff revision 1) > + { > + for (auto& module : aModulesLoaded) { > + module.mFullPath = GetModuleFullPath(module.mBase); > + bool foundInHistory = false; > + for (auto& h : mModuleHistory) { > + if (wcscmp(h.get(), module.mFullPath.get()) == 0) { Use `operator!` instead of `== 0` ::: mozglue/build/WindowsDllServices.h:45 (Diff revision 1) > +}; > + > +MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(ModuleTrustFlags); > + > +// A C-friendly interface to ModuleLoadEvent > +struct MLE_ModuleInfo_C Drop the `_C` suffixes please ::: mozglue/build/WindowsDllServices.h:173 (Diff revision 1) > virtual void DispatchDllLoadNotification(PCUNICODE_STRING aDllName) override {} > + virtual bool BeginNotifyUML(size_t aEventCount, > + int aMissedEvents, > + int aHitEvents) override { return false; } > + virtual void NotifyUML(const ModuleLoadEvent_C* event) override {} > + virtual void EndNotifyUML() override { } Nit: no space in `{ }`

Comment on attachment 8993644 [details] Bug 1435827 part 1/3: Send DLL load event info to xul; https://reviewboard.mozilla.org/r/258324/#review268128 ::: mozglue/misc/RecursiveLock_windows.h:16 (Diff revision 1) > + > +#include "mozilla/Types.h" > + > +namespace mozilla { > + > +class RecursiveLock Let's just put this code in MozglueUtils.h for now. I'd prefer that we just use plain old mozilla::Mutex on the XUL side. ::: mozglue/misc/RecursiveLock_windows.h:19 (Diff revision 1) > +namespace mozilla { > + > +class RecursiveLock > +{ > +public: > + class AutoRelease This interface is a bit confusing to me. Is it possible to refactor this so that AutoRelease works like our other RAII lock classes? You could make the methods on RecursiveLock private and then declare AutoRelease as a friend of RecursiveLock. Essentially all operations would be initiated via `AutoRelease`: e.g. ```c++ AutoRelease foo(lock); bool entered = foo.TryEnter(); ``` ::: mozglue/misc/RecursiveLock_windows.h:54 (Diff revision 1) > + AutoRelease(AutoRelease&& rhs) = default; > + > + explicit operator bool() { return mIsLocked; } > + }; > + > + CRITICAL_SECTION mLock; Make this private ::: mozglue/misc/RecursiveLock_windows.h:58 (Diff revision 1) > + > + CRITICAL_SECTION mLock; > + > + // Tracks recursion > + bool& Entered() { > + thread_local bool mEntered = false; This should be a private instance variable. Please move out of function scope and also use MOZ_THREAD_LOCAL machinery as I have outlined elsewhere. ::: mozglue/misc/RecursiveLock_windows.h:83 (Diff revision 1) > + > + AutoRelease TryEnter() { > + if (Entered()) { > + return AutoRelease(*this, false); // Don't allow recursive entry > + } > + if (TryEnterCriticalSection(&mLock) == 0) { Use `!` instead of `== 0` ::: toolkit/xre/WinDllServices.cpp:55 (Diff revision 1) > obsServ->NotifyObservers(nullptr, topic, aDllName.get()); > } > > -} // namespace mozilla > +bool > +DllServices::BeginNotifyUML(size_t aEventCount, int aMissedEvents, int aHitEvents) > +{ I know you're fleshing this out in a subsequent patch, but as long as we're stubbing this out, let's include an appropriate return statement.

Comment on attachment 8993644 [details] Bug 1435827 part 1/3: Send DLL load event info to xul; https://reviewboard.mozilla.org/r/258324/#review268144 ::: mozglue/build/UntrustedDllsProcessor.cpp:449 (Diff revision 1) > + > + > +void > +UntrustedDllsProcessorImpl::_FlushEvents(detail::DllServicesBase* aSvc) > +{ > + if (mModuleLoadEvents.empty()) { We could significantly reduce the time spent inside a lock and significantly simplify the interface between mozglue and xul. You could do something like this: ```c++ Vector<ModuleLoadEvent> other; { AutoLock lock(mLock); mModuleLoadEvents.swap(other); } aSvc->NotifyUML(other, missed, hit); ``` And then DLL services processes the vector in its entirety.

Comment on attachment 8993645 [details] Bug 1435827 part 2/3: Evaluate untrusted DLLs; https://reviewboard.mozilla.org/r/258326/#review268132 I'd like to see this again based on my issues here, as well how my suggested changes in part 1 affect this patch. ::: toolkit/xre/WinDllServices.h:17 (Diff revision 1) > +#include "mozilla/Vector.h" > > namespace mozilla { > > +// XUL-friendly ModuleLoadEvent > +struct ModuleLoadEvent_XUL Drop the `_XUL` suffix here. We can use the `glue` namespace to differentiate between the structures. ::: toolkit/xre/WinDllServices.h:25 (Diff revision 1) > + { > + uintptr_t mBase; > + nsString mLdrName; > + nsString mFullPath; > + mozilla::glue::ModuleTrustFlags mTrustFlags; > + nsString mDirectory; I think that for storage purposes, we should hold off on splitting the path into directory and leafname until we're processing the saved events. I think you should remove `mFullPath`, `mDirectory`, and `mLeafName` from this stucture and just replace it with a single `nsCOMPtr<nsIFile>` that you can extract information from later. ::: toolkit/xre/WinDllServices.cpp:18 (Diff revision 1) > #include "mozilla/ClearOnShutdown.h" > +#include "mozilla/CombinedStacks.h" > +#include "mozilla/RecursiveLock_windows.h" > #include "mozilla/Services.h" > +#include "mozilla/ScopeExit.h" > +#include "mozilla/glue/WindowsDllServices.h" You don't need to include glue/WindowsDllServices.h here; we already do that in the mozilla/WinDllServices.h header. ::: toolkit/xre/WinDllServices.cpp:39 (Diff revision 1) > + mIsStartup = aOther.mIsStartup; > + mThreadID = aOther.mThreadID; > + mProcessUptimeMS = aOther.mProcessUptimeMS; > + mStack.clear(); > + for (auto& x : aOther.mStack) { > + if (!mStack.append(x)) { `Unused` ::: toolkit/xre/WinDllServices.cpp:46 (Diff revision 1) > + } > + } > + mModules.clear(); > + if (aCopyModules) { > + for (auto& x : aOther.mModules) { > + if (!mModules.append(x)) { `Unused` ::: toolkit/xre/WinDllServices.cpp:53 (Diff revision 1) > + } > + } > + } > +} > + > +static RecursiveLock* sUntrustedModuleLoadEventsLock; Please just use a `StaticAutoPtr<mozilla::Mutex>` for this side. If that creates strange reentrancy conditions, I would rather that we resolved those in mozglue before fowarding the calls to XUL. ::: toolkit/xre/WinDllServices.cpp:55 (Diff revision 1) > + } > +} > + > +static RecursiveLock* sUntrustedModuleLoadEventsLock; > + > +class VersionInfo { Nit: brace on new line ::: toolkit/xre/WinDllServices.cpp:64 (Diff revision 1) > + nsString mLegalCopyright; > + nsString mFileDescription; > + > + class VersionNumber { > + public: > + uint64_t mVersion64; Make this private ::: toolkit/xre/WinDllServices.cpp:91 (Diff revision 1) > + > + VersionNumber mFileVersion; > + VersionNumber mProductVersion; > + > + // Returns false if it has no version resource or has no fixed version info. > + bool GetFromImage(const nsString& path) { Nit: brace on new line ::: toolkit/xre/WinDllServices.cpp:92 (Diff revision 1) > + VersionNumber mFileVersion; > + VersionNumber mProductVersion; > + > + // Returns false if it has no version resource or has no fixed version info. > + bool GetFromImage(const nsString& path) { > + DWORD infoSize = GetFileVersionInfoSizeW(path.get(), NULL); Use `nullptr` instead of `NULL`, please. ::: toolkit/xre/WinDllServices.cpp:113 (Diff revision 1) > + } > + > + wchar_t *lpBuffer; > + UINT dwBytes; > + if (::VerQueryValueW(verInfo.get(), > + L"\\StringFileInfo\\040904b0\\CompanyName", I'm concerned about the language and code page being hardcoded here. Is this identifier always present? If so, I think the magic number could use at least a comment. If not, I think we should be enumerating the available languages and choosing the one that is the best fit. ::: toolkit/xre/WinDllServices.cpp:142 (Diff revision 1) > +static void > +SplitPath(const nsString& aFullPath, nsString& aDirectory, nsString& aLeaf) > +{ > + aDirectory.Truncate(); > + aLeaf.Truncate(); > + RefPtr<nsLocalFile> f = new nsLocalFile(aFullPath); This should use [`NS_NewLocalFile`](https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Core_functions/NS_NewLocalFile) like so: ```c++ nsresult rv; nsCOMPtr<nsIFile> file; rv = NS_NewLocalFile(aFullPath, false, getter_AddRefs(file); if (NS_FAILED(rv)) { // ... } ``` ::: toolkit/xre/WinDllServices.cpp:179 (Diff revision 1) > + return ret; > +} > + > +// On error, returns partial results > +static void > +GetKeyboardLayoutDlls(mozilla::Vector<nsString>& ret) { opening brace on new line ::: toolkit/xre/WinDllServices.cpp:216 (Diff revision 1) > + } > +} > + > +// Failures should return true. > +// This also fills in some more fields in ModuleInfo > +bool How about you make this a Maybe<bool>, and then failures can be differentiated. ::: toolkit/xre/WinDllServices.cpp:297 (Diff revision 1) > + } > + } > + } > + > + if (score < 100) { > + nsAutoString orgName; If you move `signedBy` so that it lives in the same scope as `orgName`, you can use a `nsDependentString` for `orgName`, thus avoiding a copy. `orgName` will then just use `signedBy` as its buffer. ::: toolkit/xre/WinDllServices.cpp:319 (Diff revision 1) > + } > + > + return (score >= 100); > +} > + > +static mozilla::Atomic<int> sMissedMLE; Let's use a 'g' prefix here instead of an 's' prefix. In new code I'm trying to only use 's' for static vars in classes. ::: toolkit/xre/WinDllServices.cpp:339 (Diff revision 1) > > +bool > +DllServices::ProcessOutstandingUMLE() > +{ > + bool ret = !mUnprocessedUMLE.empty(); > + if (!ret) { This logic is confusing because of the double negative. ::: toolkit/xre/WinDllServices.cpp:354 (Diff revision 1) > + // Weed out any trusted modules. > + // Re-construct these two lists as we filter by trust level. > + for (auto& m : e.mModules) { > + if (!IsModuleTrusted(m, eventCopy)) { > + ModuleLoadEvent_XUL::ModuleInfo miCopy = m; > + if (!eventCopy.mModules.append(std::move(miCopy))) { `Unused` ::: toolkit/xre/WinDllServices.cpp:364 (Diff revision 1) > + > + if (eventCopy.mModules.empty()) { > + continue; > + } > + > + if (!eventsToSave.append(std::move(eventCopy))) { `Unused` ::: toolkit/xre/WinDllServices.cpp:490 (Diff revision 1) > } > > void > DllServices::NotifyUML(const glue::ModuleLoadEvent_C* event) > { > + sPendingItems --; space before `--` ::: toolkit/xre/WinDllServices.cpp:493 (Diff revision 1) > DllServices::NotifyUML(const glue::ModuleLoadEvent_C* event) > { > + sPendingItems --; > + ModuleLoadEvent_XUL eventCopy; > + > + eventCopy.mIsStartup = event->mIsStartup; I think these lines copying from the glue struct to the XUL struct might be better suited to `Assign` method or `operator=` on `ModuleLoadEvent_XUL`. ::: toolkit/xre/WinDllServices.cpp:498 (Diff revision 1) > + eventCopy.mIsStartup = event->mIsStartup; > + eventCopy.mThreadID = event->mThreadID; > + eventCopy.mProcessUptimeMS = event->mProcessUptimeMS; > + > + for (size_t i = 0; i < event->mStackSize; ++ i) { > + if (!eventCopy.mStack.emplaceBack(event->mStack[i])) { `Unused` ::: toolkit/xre/WinDllServices.cpp:503 (Diff revision 1) > + if (!eventCopy.mStack.emplaceBack(event->mStack[i])) { > + // ignore > + } > + } > + > + for (size_t i = 0; i < event->mModulesLength; ++ i) { space before `++` ::: toolkit/xre/WinDllServices.cpp:509 (Diff revision 1) > + auto& m = event->mModulesArray[i]; > + ModuleLoadEvent_XUL::ModuleInfo modCopy; > + modCopy.mBase = m.mBase; > + modCopy.mLdrName = m.mLdrName; > + modCopy.mFullPath = m.mFullPath; > + if (!eventCopy.mModules.emplaceBack(std::move(modCopy))) { `Unused` ::: toolkit/xre/WinDllServices.cpp:521 (Diff revision 1) > + if (threadName) { > + eventCopy.mThreadName = threadName; > + } > + } > + > + if (!mUnprocessedUMLE.emplaceBack(std::move(eventCopy))) { `Unused`

(In reply to François Marier [:francois] from comment #18) > Comment on attachment 8993641 [details] > Request for data collection review form - Bug 1435827.txt > > > Please see document summarizing the ongoing discussion of this feature @ > > https://docs.google.com/document/d/1-RwFbX6sNQOSU9JhKeNn9oRK17pMReHqrxHypcpyFiI/edit?usp=sharing > > That document is not publicly available. Should be public now; please check. > Is it possible that the stack traces can include user data? Stack traces contain only PDB names, the PDB unique ID, and stack frame instruction pointers. No PII. > One thing that comes to mind in terms of leaking personal information is > that the paths could include the username of the logged in user (e.g. > C:\Users\<your user name>\...). Are you doing any filtering there to try and > remove user details? Could we whitelist the directories we care about and > send a code for each of them (e.g. "system32", "firefox", ...)? Good point; we can reuse the technique employed by reporting AppInitDLLs, which strips paths unless they're whitelisted. At the moment we're whitelisting %PROGRAMFILES% and %TEMP%. Updated the attached request doc.

Comment on attachment 8999590 [details] 20180813datareview.txt 1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate? Yes, in this data review request. 2) Is there a control mechanism that allows the user to turn the data collection on and off? Yes, telemetry setting. 3) If the request is for permanent data collection, is there someone who will monitor the data over time?** Yes, :ccorcoran. 4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under? ** Category 1. 5) Is the data collection request for default-on or default-off? Default ON. 6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? No, given the filtering that is done around paths. 7) Is the data collection covered by the existing Firefox privacy notice? Yes. 8) Does there need to be a check-in in the future to determine whether to renew the data? No, permanent.

Depends on D4764

Depends on D4765

Depends on D4766

Depends on D4767

Depends on D4768

Comment on attachment 9005631 [details] Bug 1435827 part 2/6: Add AutoLock and AutoTryLock to CrossProcessSemaphore;r=jld Jed Davis [:jld] (⏰UTC-6) has approved the revision.

Depends on D6237

Depends on D6238

Depends on D6239

Depends on D6240

Depends on D6241

Depends on D6242

Depends on D6243

Comment on attachment 9010184 [details] Bug 1435827 part 6/9: ctypes tests no longer depend on successful file deletion Bobby Holley (:bholley) has approved the revision.

Comment on attachment 9010179 [details] Bug 1435827 part 2/8: Add AutoLock and AutoTryLock to CrossProcessSemaphore;r=jld Jed Davis [:jld] (⏰UTC-6) has approved the revision.

Comment on attachment 9005631 [details] Bug 1435827 part 2/6: Add AutoLock and AutoTryLock to CrossProcessSemaphore;r=jld Jed Davis [:jld] (⏰UTC-6) has been removed from the revision.

Depends on D6244

Comment on attachment 9010186 [details] Bug 1435827 part 8/9: Adding untrusted modules telemetry ping and documentation Jan-Erik Rediger [:janerik] has approved the revision.

Comment on attachment 9012879 [details] Bug 1435827 part 9/9: Adding basic test for untrusted modules ping;r=janerik Jan-Erik Rediger [:janerik] has approved the revision.

Comment on attachment 9010180 [details] Bug 1435827 part 2/9: Overload GetStackAndModules to facilitate repeated calls Jan-Erik Rediger [:janerik] has approved the revision.

Aaron, I have a couple questions spawning from recent discussions: 1. How do we want to place a lock around DllServices from XUL-land? In this case, in a telemetry background thread calling GetBinaryOrgName(). Would you say it's OK to proceed in the same way as https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/toolkit/components/telemetry/core/Telemetry.cpp#862 ? Can we guarantee this object doesn't get invalidated during use? 2. You have mentioned in https://phabricator.services.mozilla.com/D6240#inline-26043 that we can use `static nsTArray<Pair<...>>` but this results in memory leaks. Shall I go back to using the heap-allocated object?

(In reply to Carl Corcoran [:ccorcoran] from comment #48) > Aaron, I have a couple questions spawning from recent discussions: > > 1. How do we want to place a lock around DllServices from XUL-land? In this > case, in a telemetry background thread calling GetBinaryOrgName(). Would you > say it's OK to proceed in the same way as > https://searchfox.org/mozilla-central/rev/ > 3d989d097fa35afe19e814c6d5bc2f2bf10867e1/toolkit/components/telemetry/core/ > Telemetry.cpp#862 ? Can we guarantee this object doesn't get invalidated > during use? No issues with locking DllServices in XUL, it's reference counted there. Just use: RefPtr<mozilla::DllServices> dllServices(mozilla::DllServices::Get()); and then call whatever you want on |dllServices|. > > 2. You have mentioned in > https://phabricator.services.mozilla.com/D6240#inline-26043 that we can use > `static nsTArray<Pair<...>>` but this results in memory leaks. Shall I go > back to using the heap-allocated object? Damn. Two options here, in order of preference: 1. Since we already know in advance how long the array needs to be, you could probably just use an AutoTArray instead of a nsTArray, in which case you can specify a size for inline storage, thus completely avoiding the heap; or 2. Go back to using heap allocation.

Pushed to try: Windows tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db26e0989e050b336f996ecc232cf8b0b6a77e5 All platforms build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40aaa58fa424ef99e3bc7d4f303aad27977efa29

Fixed all platforms build try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd1a9a40b15b76b12d0f55134044cdcac2162de

A wrapper for ::GetModuleFileNameW() is needed in multiple places for the purpose of evaluating and processing untrusted DLLs. This adds the appropriate wrapper to WinUtils.cpp Depends on D6240

A couple of requests re: Phabricator: * Please close D6237, as it still shows up on my dashboard as needing review * Please use the Phabricator web interface to clean up the patch descriptions, as I believe that some of them are outdated.

aklotz: done! Working now on your review comments.

BTW, if you take a look at how your patch series looks in Lando (https://lando.services.mozilla.com/D7177/), you'll see that the various dependencies in the commit series don't look right. In my patches I've managed to fix this by using the web interface, going to "Edit Revision," and then ensuring that the summary field contains "Depends on DXXXXX" where XXXXX is the correct revision. Be sure to correct these dependencies before flagging for checkin.

Here's a link to mega try run with r+'d patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ddff018deca73a7665283ee67a587ef05b9a5d0 Xpcshell tests are orange due to `toolkit/modules/tests/xpcshell/test_CreditCard.js` which fails on all builds/platforms; this is almost certainly unrelated. Submitting fixed nits & fixing commit dependencies now.

Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/484638cdbc6b part 1/9: Send DLL load event info to xul;r=aklotz https://hg.mozilla.org/integration/autoland/rev/a5eb2ea37a75 part 2/9: Overload GetStackAndModules to facilitate repeated calls r=janerik https://hg.mozilla.org/integration/autoland/rev/360adbf4f1a2 part 3/9: Add WinUtils::PreparePathForTelemetry r=aklotz https://hg.mozilla.org/integration/autoland/rev/29d24c8a17ef part 4/9: Adding GetModuleFullPath to WinUtils.cpp;r=aklotz https://hg.mozilla.org/integration/autoland/rev/54e3becb215a part 5/9: Add untrusted DLLs evaluation and processing r=aklotz https://hg.mozilla.org/integration/autoland/rev/5a2f826c9f8d part 6/9: ctypes tests no longer depend on successful file deletion r=bholley https://hg.mozilla.org/integration/autoland/rev/d07fae40ddb8 part 7/9: Adding untrusted modules telemetry framework r=aklotz https://hg.mozilla.org/integration/autoland/rev/b08fe7d8cc74 part 8/9: Adding untrusted modules telemetry ping and documentation r=janerik https://hg.mozilla.org/integration/autoland/rev/ce0785970a20 part 9/9: Adding basic test for untrusted modules ping;r=janerik

https://hg.mozilla.org/mozilla-central/rev/484638cdbc6b https://hg.mozilla.org/mozilla-central/rev/a5eb2ea37a75 https://hg.mozilla.org/mozilla-central/rev/360adbf4f1a2 https://hg.mozilla.org/mozilla-central/rev/29d24c8a17ef https://hg.mozilla.org/mozilla-central/rev/54e3becb215a https://hg.mozilla.org/mozilla-central/rev/5a2f826c9f8d https://hg.mozilla.org/mozilla-central/rev/d07fae40ddb8 https://hg.mozilla.org/mozilla-central/rev/b08fe7d8cc74 https://hg.mozilla.org/mozilla-central/rev/ce0785970a20

\o/ woot!

Pull request for adding telemetry pipeline schemas: https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/215

FYI the data is flowing in and available on stmo. I whipped up 2 quick queries showing most frequently seen untrusted modules, how frequently they're seen loaded at startup (versus during runtime), and the trust flags. Trust flags are documented at https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/untrusted-modules-ping.html#payload-events-modules-moduletrustflags "Frequent Untrusted Modules in the past week (by leaf)" https://sql.telemetry.mozilla.org/queries/60448 Where one row = one DLL. "Frequent Untrusted Modules in the past week (by full path)" https://sql.telemetry.mozilla.org/queries/60451 This is similar, except grouped by the full path. The plus: the path itself gives a lot of clue about what the DLL is. The minus: grouping by full path is not as accurate as by leaf so it's noisier. Initial observation: By far the most frequent is mozglue.dll, which I suspect is due to test code here: https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/toolkit/xre/ModuleEvaluator_windows.cpp#201 I will need to adjust this in order to reduce the noise. So far I haven't looked into the stack data, other than to see that it appears to be populated and useful.

I added another redash query as well: "Primitive stack trace for a given untrusted module" https://sql.telemetry.mozilla.org/queries/60463 This takes a user-entered dll name ("comctl32.dll" is there by default), and outputs the stack traces seen for loading it. It's not symbolicated, so it's of limited practical use, but it's a step closer to a view into the stack data and suggests that incoming data is sane.