Closed
Bug 1443411
Opened 7 years ago
Closed 6 years ago
Write regression tests for 1435816
Categories
(Core :: General, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: ccorcoran, Assigned: ccorcoran)
References
(Blocks 1 open bug)
Details
(Whiteboard: inj+)
Attachments
(1 file)
In order to test bug 1435816, we'll need to launch a separate process that tries to inject a DLL into Firefox, confirming that it was blocked.
This, being the first InjectEject test, may establish techniques to be used in eventual InjectEject-style tests.
Two techniques being considered:
1. gtest, which would launch the separate injection app
2. xpcshell test, borrowing techniques from the Updater tests
Updated•7 years ago
|
Status: NEW → ASSIGNED
Whiteboard: inj? → inj+
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8987461 [details]
Bug 1443411: Add gtests for blocking threads with LoadLibrary start address;
https://reviewboard.mozilla.org/r/252700/#review260178
Mostly nits, but I'd like to see it one more time before r+.
::: mozglue/build/WindowsDllBlocklist.h:12
(Diff revision 1)
> #define mozilla_windowsdllblocklist_h
>
> #if (defined(_MSC_VER) || defined(__MINGW32__)) && (defined(_M_IX86) || defined(_M_X64))
>
> #include <windows.h>
> +#ifdef ENABLE_TESTS
Where is this macro set? By the gtest harness?
::: mozglue/build/WindowsDllBlocklist.cpp:412
(Diff revision 1)
> + gCreateThreadHook(aWasAllowed, aStartAddress);
> + }
> +}
> +
> +#else // ENABLE_TESTS
> +#define CallDllLoadHook(args...)
I think these macros should just be:
#define CallDllLoadHook(...)
(no args necessary)
::: mozglue/tests/gtest/Injector/Injector.cpp:29
(Diff revision 1)
> +#else
> + void* startAddr = (void*)strtoul(argv[2], 0, 0);
> + void* threadParam = (void*)strtoul(argv[3], 0, 0);
> +#endif
> + HANDLE targetProc = OpenProcess(PROCESS_CREATE_THREAD | PROCESS_QUERY_INFORMATION |
> + PROCESS_VM_OPERATION | PROCESS_VM_WRITE |PROCESS_VM_READ,
Nit: space between | and PROCESS_VM_READ
::: mozglue/tests/gtest/TestDLLEject.cpp:29
(Diff revision 1)
> +#define DLL_LEAF_NAME (u"InjectorDLL.dll")
> +
> +static nsString
> +makeString(PUNICODE_STRING aOther)
> +{
> + size_t chars = aOther->Length / 2;
Please rename to numChars and s/2/sizeof(WCHAR) please
::: mozglue/tests/gtest/TestDLLEject.cpp:61
(Diff revision 1)
> + SetEvent(sThreadWasAllowed);
> + }
> + }
> +}
> +
> +template<typename TgetArgsProc>
Can you please add some comments at the beginning of major blocks in this function to describe how it works?
That would be helpful to a reader who is less familiar with what we're trying to test here than you or me.
::: mozglue/tests/gtest/TestDLLEject.cpp:65
(Diff revision 1)
> +
> +template<typename TgetArgsProc>
> +static void
> +DoTest_CreateRemoteThread_LoadLibrary(TgetArgsProc aGetArgsProc)
> +{
> + sTestRunning ++;
Style nit: no space before increment/decrement
::: mozglue/tests/gtest/TestDLLEject.cpp:96
(Diff revision 1)
> + STARTUPINFOW si = { 0 };
> + si.cb = sizeof(si);
> + ::GetStartupInfoW(&si);
> + PROCESS_INFORMATION pi = { 0 };
> +
> + nsString path = nsString(u"Injector.exe");
Just use
nsString path(u"Injector.exe");
::: mozglue/tests/gtest/TestDLLEject.cpp:98
(Diff revision 1)
> + ::GetStartupInfoW(&si);
> + PROCESS_INFORMATION pi = { 0 };
> +
> + nsString path = nsString(u"Injector.exe");
> + nsString dllPath = nsString(DLL_LEAF_NAME);
> + // {
I assume this comment block is vesigial? Please remove.
::: mozglue/tests/gtest/TestDLLEject.cpp:146
(Diff revision 1)
> + sThreadWasBlocked,
> + sThreadWasAllowed,
> + sDllWasLoaded,
> + pi.hProcess
> + };
> + int handleCount = ARRAYSIZE(handles);
Please use mozilla::ArrayLength instead (via mozilla/ArrayUtils.h)
::: mozglue/tests/gtest/TestDLLEject.cpp:153
(Diff revision 1)
> +
> + while(keepGoing) {
> + switch(WaitForMultipleObjectsEx(handleCount, handles,
> + FALSE, sTimeoutMS, FALSE)) {
> + case WAIT_OBJECT_0: {
> + EXPECT_TRUE("Thread was blocked successfully.");
Nit: indentation
::: mozglue/tests/gtest/TestDLLEject.cpp:198
(Diff revision 1)
> + }
> + }
> +
> + // Double-check that injectordll is not loaded.
> + auto hExisting = GetModuleHandleW(dllPath.get());
> + EXPECT_TRUE(hExisting == 0 || hExisting == INVALID_HANDLE_VALUE);
You don't need to check for INVALID_HANDLE_VALUE with HMODULEs.
Attachment #8987461 -
Flags: review?(aklotz) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8987461 [details]
Bug 1443411: Add gtests for blocking threads with LoadLibrary start address;
https://reviewboard.mozilla.org/r/252700/#review260178
> Where is this macro set? By the gtest harness?
This is set by the build config (ac_add_options --disable-tests -> OBJ/mozilla-config.h) documented here https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Configuring_Build_Options. I tested that this config works locally, though I just assume builds-for-release disable this macro.
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8987461 [details]
Bug 1443411: Add gtests for blocking threads with LoadLibrary start address;
https://reviewboard.mozilla.org/r/252700/#review260178
New revision addresses all the issues you found, and simplifies the control flow.
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8987461 [details]
Bug 1443411: Add gtests for blocking threads with LoadLibrary start address;
https://reviewboard.mozilla.org/r/252700/#review262702
Great, thanks!
Attachment #8987461 -
Flags: review?(aklotz) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/130e3b2dd716
Add gtests for blocking threads with LoadLibrary start address;r=aklotz
Keywords: checkin-needed
Comment 8•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•