This could allow us to do interception in a suspended child process (for bootstrapping).

I have a WIP patch for this, so I'll take it.

FYI, this review is ... kind of hard. I really thought I'd have it today but I won't. I'm pretty certain I'll have it tomorrow -- I'll say I'm about 85% of the way through. Nothing very big so far. I should mention that I can't apply the patches cleanly -- there are a few conflicts but they are basically whole-file sized. I don't know why this is. Your patch says that it is based off of commit deb7714a7bcd3448952440e92d0209abec6b886d which is from Tue Mar 13 19:01:32 2018. I tried applying the patch to that commit and I still get conflicts. This hasn't really impeded the review tho.

I can push a rebased version if you want...

Comment on attachment 8965508 [details] Bug 1432653: Refactor the DLL interceptor and parameterize its memory operations; https://reviewboard.mozilla.org/r/234304/#review241990 This is my first mozreview attempt (I never had a problem with bugzilla's interface but that's not a common opinion). If I screwed something up, let me know. I find this interface very unintuitive. Overall, the patch looks great. Mostly, I've got nits and some clarification requests. ::: mozglue/build/moz.build:58 (Diff revision 3) > '/memory/build', > ] > > if CONFIG['CC_TYPE'] == "msvc": > SOURCES += ['WindowsCFGStatus.cpp'] > + nit - newline ::: mozglue/misc/interceptor/MMPolicies.h:118 (Diff revision 3) > + { > + ::memcpy(aToPtr, aFromPtr, aLen); > + return true; > + } > + > + template <typename T> Just pointing out that this is (currently) unused. Keep it if you like (but less generic code is always nice). ::: mozglue/misc/interceptor/TargetFunction.h:32 (Diff revision 3) > + , mFunc(aFunc) > + , mNumBytes(aNumBytes) > + , mOffset(0) > + , mStartWriteOffset(0) > + , mPrevProt(0) > + , mAccumulatedStatus(true) What is mAccumulatedStatus for? I couldn't figure it out. ::: mozglue/misc/interceptor/TargetFunction.h:141 (Diff revision 3) > + } > + > + mOffset += sizeof(uint16_t); > + } > + > + void WriteDisplacement(const uintptr_t aAbsTarget) nit: You have a few functions related to displacements that are 32-bit specific. I think it helps to call e.g. this one WriteDisplacement32 (since we encounter 8/16 bit displacements as well). ::: mozglue/misc/interceptor/TargetFunction.h:174 (Diff revision 3) > + > + mOffset += sizeof(uintptr_t); > + } > + > + template <typename T, size_t M, size_t N> > + bool VerifyValuesAreOneOf(const T (&aValues)[N], const uint8_t aOffset = 0) Could use docs. Something like "True if M values of type T taken from the function are all also in aValues" ::: mozglue/misc/interceptor/TargetFunction.h:236 (Diff revision 3) > + : mMMPolicy(aOther.mMMPolicy) > + , mBase(aOther.mBase) > + { > + } > + > + ReadOnlyTargetBytes(const ReadOnlyTargetBytes& aOther) nit - This can be removed if you give the next constructor a default offset of 0. ::: mozglue/misc/interceptor/TargetFunction.h:249 (Diff revision 3) > + : mMMPolicy(aOther.mMMPolicy) > + , mBase(aOther.mBase + aOffsetFromOther) > + { > + } > + > + void EnsureLimit(uint32_t aDesiredLimit) I'm not clear on this but it looks like you've wired this up anticipating that you're going to use it in the future. This is the only EnsureLimit impl (ReadOnlyTargetBytes only has the one template specialization implementation). It looks like its used correctly (assuming it works like reserve in STL containers). I am also unclear on _why_ its a no op here. ::: mozglue/misc/interceptor/TargetFunction.h:276 (Diff revision 3) > + > +template <typename MMPolicy> > +class MOZ_STACK_CLASS ReadOnlyTargetFunction final > +{ > + template <typename MMPolicy> > + class TargetBytesPtr TargetBytesPtr can get MMPolicy from the outer ReadOnlyTargetFunction class. If there is a future plan that requires them to be different templates then we should keep both templates but I'd then rename the one in TargetBytesPtr to avoid shadowing. ::: mozglue/misc/interceptor/TargetFunction.h:286 (Diff revision 3) > + static Type Make(const MMPolicy& aMMPolicy, const void* aFunc) > + { > + return Move(TargetBytesPtr(aMMPolicy, aFunc)); > + } > + > + static Type CopyFromOffset(const TargetBytesPtr& aOther, IIUC, CopyFromOffset() calls TargetBytesPtr(const TargetBytesPtr& aOther,const uint32_t aOffsetFromOther) which calls ReadOnlyTargetFunction(const ReadOnlyTargetFunction& aOther,const uint32_t aOffsetFromOther) which calls CopyFromOffset() == infinite loop (although I don't see that these methods are currently called from outside of this circular chain) ::: mozglue/misc/interceptor/TargetFunction.h:410 (Diff revision 3) > + uint32_t GetOffset() const > + { > + return mOffset; > + } > + > + uintptr_t ReadDispAsAbsolute() nit: Another Disp -> Disp32 (or could make it robust to other sizes). ::: mozglue/misc/interceptor/TargetFunction.h:434 (Diff revision 3) > + * to zero, this object's current offset is used as the length. > + * @param aOffset The result's base address will be offset from this > + * object's base address by |aOffset| bytes. This value may be > + * negative. > + */ > + WritableTargetFunction<MMPolicy> Promote(const uint32_t aLen = 0, Is there a way to make this safer by checking that the offset won't take us out of the allocation? ::: mozglue/misc/interceptor/Trampoline.h:149 (Diff revision 3) > + if (mOffset + sizeof(int32_t) > mMaxOffset) { > + mAccumulatedStatus = false; > + return; > + } > + > + // This needs to be computed from the remote location I don't get this but I'm assuming there is more to come in the future. Why is this remote-proc-relative but e.g. WriteableTargetFunction::WriteDisplacement is "local"-relative? ::: mozglue/misc/interceptor/VMSharingPolicies.h:27 (Diff revision 3) > + : MMPolicy(mozilla::Forward<Args>(aArgs)...) > + , mNextChunkIndex(0) > + { > + } > + > + bool Reserve(uint32_t aCount) This essentially fails if it was called before (it ignores the count you request). The pre-existing size might not be sufficient. You pretty clearly don't do that but a MOZ_ASSERT of that wouldn't hurt. ::: mozglue/misc/moz.build:39 (Diff revision 3) > > if CONFIG['OS_ARCH'] == 'WINNT': > + EXPORTS += [ > + 'nsWindowsDllInterceptor.h', > + ] > + EXPORTS.mozilla.interceptor += [ Do these mozilla.interceptor exports need to be exported? That may be more future plans but, in this patch, they are local to the stuff in interceptor, except nsWindowsDllInterceptor.h, which is just in the parent folder so it can reach it relatively. ::: mozglue/misc/interceptor/PatcherBase.h:62 (Diff revision 3) > - } > - > -#if defined(_M_IX86) > // If function entry is jmp [disp32] such as used by kernel32, > // we resolve redirected address from import table. > - if (aOriginalFunction[0] == 0xff && aOriginalFunction[1] == 0x25) { > + if (origFn[0] == 0x48 && origFn[1] == 0xff && origFn[2] == 0x25) { Does x64 put 0x48 in jmp table commands? I dont see it in the old interceptor code. 0x48 is a REX.W, which is ignored with jmp (see https://stackoverflow.com/questions/36788685/meaning-of-rex-w-prefix-before-amd64-jmp-ff25). ::: mozglue/misc/interceptor/PatcherDetour.h:152 (Diff revision 3) > + // might as well utilize that entire 64KiB reservation instead of > + // artifically constraining ourselves to the page size. > + aNumHooks = mVMPolicy.GetAllocGranularity() / kHookSize; > } > > - mMaxHooks = aNumHooks + (hooksPerPage % aNumHooks); > + mVMPolicy.Reserve(aNumHooks); Is it safe to ignore the return error code here? ::: mozglue/misc/interceptor/PatcherDetour.h:414 (Diff revision 3) > + tramp.WriteEncodedPointer(this); > if (!tramp) { > return; > } > > - // We keep the address of the original function in the first bytes of > + auto clearInstanceOnFailure = MakeScopeExit([aOutTramp, &tramp]() -> void { Would benefit from a comment that clearInstanceOnFailure works because aOutTramp isn't written to until the function is successful. ::: mozglue/misc/interceptor/PatcherDetour.h:580 (Diff revision 3) > + } > } else { > MOZ_ASSERT_UNREACHABLE("Unrecognized opcode sequence"); > return; > } > - } else if (origBytes[nOrigBytes] == 0x40 || > + } else if (*origBytes >= 0x88 && *origBytes <= 0x8B) { I see an x86 version of the 0x88-0x8b operations in the old code but not x64 versions. Is this intentional? (I haven't checked the x64 docs but its probably valid. I'll look them up if you meant to add this.) ::: mozglue/misc/interceptor/PatcherNopSpace.h:103 (Diff revision 3) > LONG status = RegQueryValueExW(hkey, kAppInitDLLs, nullptr, > nullptr, nullptr, &numBytes); > mozilla::UniquePtr<wchar_t[]> data; > if (!status) { > // Allocate the buffer and query for the actual data > - data = mozilla::MakeUnique<wchar_t[]>(numBytes / sizeof(wchar_t)); > + data = mozilla::MakeUnique<wchar_t[]>((numBytes + 1) / sizeof(wchar_t)); This change doesn't look necessary

Comment on attachment 8965508 [details] Bug 1432653: Refactor the DLL interceptor and parameterize its memory operations; https://reviewboard.mozilla.org/r/234304/#review243124

mozreview is mixing together my old comments and my new ones so I'm skipping it and doing these comments manually. Its also possible I missed something since mozreview did not handle the diffs well so I'm also going off a try push. > ::: mozglue/misc/interceptor/TargetFunction.h:32 > (Diff revision 3) > > + , mFunc(aFunc) > > + , mNumBytes(aNumBytes) > > + , mOffset(0) > > + , mStartWriteOffset(0) > > + , mPrevProt(0) > > + , mAccumulatedStatus(true) > > What is mAccumulatedStatus for? I couldn't figure it out. I still don't get this. > ::: mozglue/misc/interceptor/TargetFunction.h:286 > (Diff revision 3) > > + static Type Make(const MMPolicy& aMMPolicy, const void* aFunc) > > + { > > + return Move(TargetBytesPtr(aMMPolicy, aFunc)); > > + } > > + > > + static Type CopyFromOffset(const TargetBytesPtr& aOther, > > IIUC, CopyFromOffset() calls > > TargetBytesPtr(const TargetBytesPtr& aOther,const uint32_t aOffsetFromOther) > which calls > > ReadOnlyTargetFunction(const ReadOnlyTargetFunction& aOther,const uint32_t > aOffsetFromOther) which calls > > CopyFromOffset() > > == infinite loop > > (although I don't see that these methods are currently called from outside > of this circular chain) This still looks circular to me. > ::: mozglue/misc/interceptor/Trampoline.h:149 > (Diff revision 3) > > + if (mOffset + sizeof(int32_t) > mMaxOffset) { > > + mAccumulatedStatus = false; > > + return; > > + } > > + > > + // This needs to be computed from the remote location > > I don't get this but I'm assuming there is more to come in the future. Why > is this remote-proc-relative but e.g. > WriteableTargetFunction::WriteDisplacement is "local"-relative? I still don't get this. > ::: mozglue/misc/moz.build:39 > (Diff revision 3) > > > > if CONFIG['OS_ARCH'] == 'WINNT': > > + EXPORTS += [ > > + 'nsWindowsDllInterceptor.h', > > + ] > > + EXPORTS.mozilla.interceptor += [ > > Do these mozilla.interceptor exports need to be exported? That may be more > future plans but, in this patch, they are local to the stuff in interceptor, > except nsWindowsDllInterceptor.h, which is just in the parent folder so it > can reach it relatively. These exports still look superfluous to me. > ::: mozglue/misc/interceptor/PatcherBase.h:62 > (Diff revision 3) > > - } > > - > > -#if defined(_M_IX86) > > // If function entry is jmp [disp32] such as used by kernel32, > > // we resolve redirected address from import table. > > - if (aOriginalFunction[0] == 0xff && aOriginalFunction[1] == 0x25) { > > + if (origFn[0] == 0x48 && origFn[1] == 0xff && origFn[2] == 0x25) { > > Does x64 put 0x48 in jmp table commands? I dont see it in the old > interceptor code. 0x48 is a REX.W, which is ignored with jmp (see > https://stackoverflow.com/questions/36788685/meaning-of-rex-w-prefix-before- > amd64-jmp-ff25). I still don't get why this is changed. > ::: mozglue/misc/interceptor/PatcherDetour.h:152 > (Diff revision 3) > > + // might as well utilize that entire 64KiB reservation instead of > > + // artifically constraining ourselves to the page size. > > + aNumHooks = mVMPolicy.GetAllocGranularity() / kHookSize; > > } > > > > - mMaxHooks = aNumHooks + (hooksPerPage % aNumHooks); > > + mVMPolicy.Reserve(aNumHooks); > > Is it safe to ignore the return error code here? This still looks unsafe to me. > ::: mozglue/misc/interceptor/PatcherDetour.h:580 > (Diff revision 3) > > + } > > } else { > > MOZ_ASSERT_UNREACHABLE("Unrecognized opcode sequence"); > > return; > > } > > - } else if (origBytes[nOrigBytes] == 0x40 || > > + } else if (*origBytes >= 0x88 && *origBytes <= 0x8B) { > > I see an x86 version of the 0x88-0x8b operations in the old code but not x64 > versions. Is this intentional? (I haven't checked the x64 docs but its > probably valid. I'll look them up if you meant to add this.) Still need an explanation for this one. > ::: mozglue/misc/interceptor/PatcherNopSpace.h:103 > (Diff revision 3) > > LONG status = RegQueryValueExW(hkey, kAppInitDLLs, nullptr, > > nullptr, nullptr, &numBytes); > > mozilla::UniquePtr<wchar_t[]> data; > > if (!status) { > > // Allocate the buffer and query for the actual data > > - data = mozilla::MakeUnique<wchar_t[]>(numBytes / sizeof(wchar_t)); > > + data = mozilla::MakeUnique<wchar_t[]>((numBytes + 1) / sizeof(wchar_t)); > > This change doesn't look necessary I still don't get this either.

I actually added a bunch of comments for this stuff in MozReview, but they appear to have disappeared into the ether. :-( I'll see if I can surface them somehow.

Comment on attachment 8965508 [details] Bug 1432653: Refactor the DLL interceptor and parameterize its memory operations; https://reviewboard.mozilla.org/r/234304/#review241990 > nit - newline Fixed. > Just pointing out that this is (currently) unused. Keep it if you like (but less generic code is always nice). Good catch! That is vestigial. Removed. > What is mAccumulatedStatus for? I couldn't figure it out. If any of the functions called on the TargetFunction or Trampoline have failed, this boolean becomes false. This way, instead of having to add noise to the code by checking a return status for every single call to Read* or Write*, we just call operator! at the end to see if everything was successful. > nit: You have a few functions related to displacements that are 32-bit specific. I think it helps to call e.g. this one WriteDisplacement32 (since we encounter 8/16 bit displacements as well). Sure, clarity is good. > Could use docs. Something like "True if M values of type T taken from the function are all also in aValues" SGTM. > I'm not clear on this but it looks like you've wired this up anticipating that you're going to use it in the future. This is the only EnsureLimit impl (ReadOnlyTargetBytes only has the one template specialization implementation). It looks like its used correctly (assuming it works like reserve in STL containers). I am also unclear on _why_ its a no op here. In a follow-up bug for out-of-process interception, EnsureLimit is essentially used to ReadProcessMemory on the target function's bytes into a local buffer so that we can read them. It's a no-op in the in-proc case because the bytes that we're reading are always present (well, barring invalid memory at the target address, but that's a separate problem). I'll clarify that comment a bit. > TargetBytesPtr can get MMPolicy from the outer ReadOnlyTargetFunction class. If there is a future plan that requires them to be different templates then we should keep both templates but I'd then rename the one in TargetBytesPtr to avoid shadowing. There will be specializations in a future patch, so I renamed it. > IIUC, CopyFromOffset() calls > > TargetBytesPtr(const TargetBytesPtr& aOther,const uint32_t aOffsetFromOther) which calls > > ReadOnlyTargetFunction(const ReadOnlyTargetFunction& aOther,const uint32_t aOffsetFromOther) which calls > > CopyFromOffset() > > == infinite loop > > (although I don't see that these methods are currently called from outside of this circular chain) TargetBytesPtr(const TargetBytesPtr& aOther,const uint32_t aOffsetFromOther) calls ReadOnlyTargetBytes (as opposed to ReadOnlyTargetFunction), so I think we're okay here. > Is there a way to make this safer by checking that the offset won't take us out of the allocation? Sure. I've added a check to see if adding the offset crosses a page boundary, and if so, it does a VirtualQuery. > I don't get this but I'm assuming there is more to come in the future. Why is this remote-proc-relative but e.g. WriteableTargetFunction::WriteDisplacement is "local"-relative? For out-of-proc interception, the trampoline will be writing to a local view of shared memory, but it needs to compute all pointer arithmetic in terms of the address of the view mapped into the target process. For in-proc interception, "local" and "remote" pointers are always identical. As for why the read-only and writable target functions don't use this concept, it is simply because they do not use shared memory with separate local and remote views for their r/w operations; they always operate on the "remote" pointer. > Do these mozilla.interceptor exports need to be exported? That may be more future plans but, in this patch, they are local to the stuff in interceptor, except nsWindowsDllInterceptor.h, which is just in the parent folder so it can reach it relatively. I don't think so. nsWindowsDllInterceptor.h is exported, so anything that includes it from dist/include needs will need its dependencies to be exported as well. > Does x64 put 0x48 in jmp table commands? I dont see it in the old interceptor code. 0x48 is a REX.W, which is ignored with jmp (see https://stackoverflow.com/questions/36788685/meaning-of-rex-w-prefix-before-amd64-jmp-ff25). I legitimately encountered this during testing. > Is it safe to ignore the return error code here? Yes, that should be safe. Trampoline creation will fail and everything will error out. > I see an x86 version of the 0x88-0x8b operations in the old code but not x64 versions. Is this intentional? (I haven't checked the x64 docs but its probably valid. I'll look them up if you meant to add this.) Yes, I encounted it during testing. It's the exact same opcodes as 32-bit mode. > This change doesn't look necessary This is a security precaution that we've been using with the "Query for size" followed by "call with buffer at that size" pattern. While unlikely, this ensures that if RegQueryValueEx produces a size such that (size % sizeof(wchar_t) != 0), we round up by one to ensure that we don't overflow the buffer. Thanks to integer rounding, it becomes redundant if the size is indeed correct.

Comment on attachment 8965508 [details] Bug 1432653: Refactor the DLL interceptor and parameterize its memory operations; https://reviewboard.mozilla.org/r/234304/#review243446 Yeah -- that's what I was missing. ::: mozglue/misc/interceptor/TargetFunction.h:35 (Diff revisions 3 - 4) > + , mNumBytes(0) > + , mOffset(0) > + , mStartWriteOffset(0) > + , mPrevProt(0) > + , mAccumulatedStatus(false) > + { ::: mozglue/misc/interceptor/TargetFunction.h:35 (Diff revisions 3 - 4) > + , mNumBytes(0) > + , mOffset(0) > + , mStartWriteOffset(0) > + , mPrevProt(0) > + , mAccumulatedStatus(false) > + { ::: mozglue/misc/interceptor/TargetFunction.h:326 (Diff revisions 3 - 4) > } > > static Type CopyFromOffset(const TargetBytesPtr& aOther, > const uint32_t aOffsetFromOther) > { > return Move(TargetBytesPtr(aOther, aOffsetFromOther)); ::: mozglue/misc/interceptor/Trampoline.h:149 (Diff revisions 3 - 4) > if (mOffset + sizeof(int32_t) > mMaxOffset) { > mAccumulatedStatus = false; > return; > } > > // This needs to be computed from the remote location ::: mozglue/misc/moz.build:39 (Diff revision 4) > > if CONFIG['OS_ARCH'] == 'WINNT': > + EXPORTS += [ > + 'nsWindowsDllInterceptor.h', > + ] > + EXPORTS.mozilla.interceptor += [ ::: mozglue/misc/interceptor/PatcherDetour.h:58 (Diff revision 4) > #endif > - return false; > - } > > - MOZ_RELEASE_ASSERT(mPatchedFnsLen < maxPatchedFns, "No room for the hook"); > - > + const auto& tramps = mVMPolicy.Items(); > + for (auto&& tramp : tramps) {

(Ignore the code references in the last comment -- I had started some notes and this was as close to deleting them as I was able to get with mozreview.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/25a807fa07d104fbf5a7796e8ea6e29ce94e9c35 Bug 1432653: Refactor the DLL interceptor and parameterize its memory operations; r=handyman