Closed Bug 1443411 Opened 2 years ago Closed 2 years ago

Write regression tests for 1435816

Categories

(Core :: General, enhancement, P2)

Unspecified
Windows
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

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
Status: NEW → ASSIGNED
Whiteboard: inj? → inj+
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 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.
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/130e3b2dd716
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.