|
|
Created:
3 years, 10 months ago by penny Modified:
3 years, 10 months ago Reviewers:
Will Harris CC:
chromium-reviews, wfh+watch_chromium.org, pennymac+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Windows CFG Test] Added unittest for CFG enabling on process.
Test ensures our build system enables CFG for a process when asked.
Ensures that an MS system DLL in the process space is protected, even if
none of Chromium code is compiled with CFG.
TEST=New test CFGSupportTests.MsIndirectFailure in sbox_integration_tests suite.
R=wfh
BUG=584575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2679793002
Cr-Commit-Position: refs/heads/master@{#450498}
Committed: https://chromium.googlesource.com/chromium/src/+/8c67aad851ba03a83144557abfb2d4b3d182095e
Patch Set 1 #Patch Set 2 : Fixing 'git cl format' breakage. #
Total comments: 12
Patch Set 3 : Code review fixes, part 1. #Patch Set 4 : Fixed swarmed test. #
Total comments: 4
Patch Set 5 : Code review fixes, part 2. #
Messages
Total messages: 29 (19 generated)
Description was changed from ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. BUG=584575 ========== to ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. BUG=584575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. BUG=584575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. TEST=New test CFGSupportTests.MsIndirectFailure in sbox_integration_tests suite. BUG=584575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by pennymac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Description was changed from ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. TEST=New test CFGSupportTests.MsIndirectFailure in sbox_integration_tests suite. BUG=584575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. TEST=New test CFGSupportTests.MsIndirectFailure in sbox_integration_tests suite. R=wfh BUG=584575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
pennymac@chromium.org changed reviewers: + wfh@chromium.org
On 2017/02/06 21:37:44, penny wrote: > mailto:pennymac@chromium.org changed reviewers: > + mailto:wfh@chromium.org Hey Will, Would you mind doing a quick review of a new CFG test, added under sandbox? I added myself to OWNERS as well - but happy to do that in separate CL if you prefer. Thanks!
The CQ bit was checked by pennymac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
A few initial comments in general I'd like to see more comments in the test file e.g. what exactly are you testing here, and why should CFG even be tripping with this patch in place? https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/BUILD.gn File sandbox/win/BUILD.gn (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/BUILD.gn#ne... sandbox/win/BUILD.gn:212: executable("cfi_unittest_exe") { hmm I wonder if this test executable should go into tests/integration_tests rather than src/ since it's test only code. I don't feel too strongly about this, but I think that the pattern would then match targets like sbox_integration_test_hook_dll and sbox_integration_test_win_proc. https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/OWNERS File sandbox/win/OWNERS (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/OWNERS#newc... sandbox/win/OWNERS:4: pennymac@chromium.org do this in a separate CL https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... File sandbox/win/src/cfi_unittest.cc (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest.cc:16: const DWORD kCfgExceptionId = 0xc0000409; This appears to be declared (in winnt.h) can it be included somehow from SDK instead of hard coded? https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest.cc:29: if (::IsWindows8Point1OrGreater()) { we would normally use base::win::GetVersion and VERSION_WIN8_1 and not this function, is there any reason not to here? https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest.cc:38: if (!proc.WaitForExitWithTimeout(base::TimeDelta::FromSeconds(5), consider using TestTimeouts::tiny_timeout() or action_timeout() https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... File sandbox/win/src/cfi_unittest_exe.cc (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest_exe.cc:19: // Tests don't run in a wow64 environment, so fine to compile based on ifdef. I'm not sure I understand this comment. We have 32-bit tests that run on 64-bit Windows. or does this comment mean something else?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Thanks! Fixes uploaded. https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/BUILD.gn File sandbox/win/BUILD.gn (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/BUILD.gn#ne... sandbox/win/BUILD.gn:212: executable("cfi_unittest_exe") { On 2017/02/06 22:33:36, Will Harris wrote: > hmm I wonder if this test executable should go into tests/integration_tests > rather than src/ since it's test only code. > > I don't feel too strongly about this, but I think that the pattern would then > match targets like sbox_integration_test_hook_dll and > sbox_integration_test_win_proc. This is a bit of a mess right now (old conventions mixed with newer standards) - especially with process_mitigations_test.cc being under sandbox\win\src\. But I agree with you, and like how I put the extra testing DLLs and EXEs under tests\integration_tests. I've moved both cfi_test* files as you suggest (I've moved them 3 times during implementation :). https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/OWNERS File sandbox/win/OWNERS (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/OWNERS#newc... sandbox/win/OWNERS:4: pennymac@chromium.org On 2017/02/06 22:33:36, Will Harris wrote: > do this in a separate CL Done. https://codereview.chromium.org/2677073008 https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... File sandbox/win/src/cfi_unittest.cc (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest.cc:16: const DWORD kCfgExceptionId = 0xc0000409; On 2017/02/06 22:33:36, Will Harris wrote: > This appears to be declared (in winnt.h) can it be included somehow from SDK > instead of hard coded? Done. https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest.cc:29: if (::IsWindows8Point1OrGreater()) { On 2017/02/06 22:33:36, Will Harris wrote: > we would normally use base::win::GetVersion and VERSION_WIN8_1 and not this > function, is there any reason not to here? Yes. If one only has to check version, and nothing more intricate, it's more efficient to use these APIs (macros). No need to include all of base/win/windows_version.h and set up the singleton. No need to call down to GetVersion/NtQuerySystemInformation for this one unit test. Microsoft have been recommending using these APIs for some time now. https://msdn.microsoft.com/en-us/library/windows/desktop/ms724832(v=vs.85).aspx These APIs also take into account manifesting/compatibility mode. However, you're right that our chrome code currently uses the base version singleton, so I totally agree that we should be consistent in that regard. If this code was in a standalone module with no base, I'd argue these APIs should be used. But sbox_integration_tests definitely already uses the base singleton. So for now, I've changed this to use base... and I'll revisit how base/win/windows_version is implemented in the future. DONE https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest.cc:38: if (!proc.WaitForExitWithTimeout(base::TimeDelta::FromSeconds(5), On 2017/02/06 22:33:36, Will Harris wrote: > consider using TestTimeouts::tiny_timeout() or action_timeout() Done. action_timeout(). I wasn't 100% certain this would always be long enough. I'm waiting on process creation, then library load and copy-on-write system dll, etc... but I'm using this timeout in process_mitigations_test.cc, so it should be fine! Thanks. https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... File sandbox/win/src/cfi_unittest_exe.cc (right): https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... sandbox/win/src/cfi_unittest_exe.cc:19: // Tests don't run in a wow64 environment, so fine to compile based on ifdef. On 2017/02/06 22:33:36, Will Harris wrote: > I'm not sure I understand this comment. We have 32-bit tests that run on 64-bit > Windows. or does this comment mean something else? Done.
On 2017/02/07 23:12:56, penny wrote: > Thanks! Fixes uploaded. > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/BUILD.gn > File sandbox/win/BUILD.gn (right): > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/BUILD.gn#ne... > sandbox/win/BUILD.gn:212: executable("cfi_unittest_exe") { > On 2017/02/06 22:33:36, Will Harris wrote: > > hmm I wonder if this test executable should go into tests/integration_tests > > rather than src/ since it's test only code. > > > > I don't feel too strongly about this, but I think that the pattern would then > > match targets like sbox_integration_test_hook_dll and > > sbox_integration_test_win_proc. > > This is a bit of a mess right now (old conventions mixed with newer standards) - > especially with process_mitigations_test.cc being under sandbox\win\src\. But I > agree with you, and like how I put the extra testing DLLs and EXEs under > tests\integration_tests. I've moved both cfi_test* files as you suggest (I've > moved them 3 times during implementation :). > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/OWNERS > File sandbox/win/OWNERS (right): > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/OWNERS#newc... > sandbox/win/OWNERS:4: mailto:pennymac@chromium.org > On 2017/02/06 22:33:36, Will Harris wrote: > > do this in a separate CL > > Done. https://codereview.chromium.org/2677073008 > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... > File sandbox/win/src/cfi_unittest.cc (right): > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... > sandbox/win/src/cfi_unittest.cc:16: const DWORD kCfgExceptionId = 0xc0000409; > On 2017/02/06 22:33:36, Will Harris wrote: > > This appears to be declared (in winnt.h) can it be included somehow from SDK > > instead of hard coded? > > Done. > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... > sandbox/win/src/cfi_unittest.cc:29: if (::IsWindows8Point1OrGreater()) { > On 2017/02/06 22:33:36, Will Harris wrote: > > we would normally use base::win::GetVersion and VERSION_WIN8_1 and not this > > function, is there any reason not to here? > > Yes. If one only has to check version, and nothing more intricate, it's more > efficient to use these APIs (macros). No need to include all of > base/win/windows_version.h and set up the singleton. No need to call down to > GetVersion/NtQuerySystemInformation for this one unit test. > > Microsoft have been recommending using these APIs for some time now. > https://msdn.microsoft.com/en-us/library/windows/desktop/ms724832(v=vs.85).aspx > These APIs also take into account manifesting/compatibility mode. > > However, you're right that our chrome code currently uses the base version > singleton, so I totally agree that we should be consistent in that regard. If > this code was in a standalone module with no base, I'd argue these APIs should > be used. But sbox_integration_tests definitely already uses the base singleton. > > > So for now, I've changed this to use base... and I'll revisit how > base/win/windows_version is implemented in the future. > > DONE > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... > sandbox/win/src/cfi_unittest.cc:38: if > (!proc.WaitForExitWithTimeout(base::TimeDelta::FromSeconds(5), > On 2017/02/06 22:33:36, Will Harris wrote: > > consider using TestTimeouts::tiny_timeout() or action_timeout() > > Done. action_timeout(). > > I wasn't 100% certain this would always be long enough. I'm waiting on process > creation, then library load and copy-on-write system dll, etc... but I'm using > this timeout in process_mitigations_test.cc, so it should be fine! Thanks. > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... > File sandbox/win/src/cfi_unittest_exe.cc (right): > > https://codereview.chromium.org/2679793002/diff/20001/sandbox/win/src/cfi_uni... > sandbox/win/src/cfi_unittest_exe.cc:19: // Tests don't run in a wow64 > environment, so fine to compile based on ifdef. > On 2017/02/06 22:33:36, Will Harris wrote: > > I'm not sure I understand this comment. We have 32-bit tests that run on > 64-bit > > Windows. or does this comment mean something else? > > Done. Forgot to mention that I added a chunk of commenting in the exe, explaining the patch a bit better. Let me know if that makes sense.
Patchset #4 (id:100001) has been deleted
this is very good test. lgtm with a few comments. https://codereview.chromium.org/2679793002/diff/120001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/cfi_unittest.cc (right): https://codereview.chromium.org/2679793002/diff/120001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/cfi_unittest.cc:14: const wchar_t* kExeFilename = L"cfi_unittest_exe.exe"; Declare variables as close to them being used as possible, so below in the function. (chromium coding standard...) https://codereview.chromium.org/2679793002/diff/120001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/cfi_unittest.cc:34: #if !defined(NDEBUG) Consider just commenting out the entire test, instead of making it pass on debug builds?
Thanks for the review Will! https://codereview.chromium.org/2679793002/diff/120001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/cfi_unittest.cc (right): https://codereview.chromium.org/2679793002/diff/120001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/cfi_unittest.cc:14: const wchar_t* kExeFilename = L"cfi_unittest_exe.exe"; On 2017/02/10 22:38:47, Will Harris wrote: > Declare variables as close to them being used as possible, so below in the > function. (chromium coding standard...) Done. https://codereview.chromium.org/2679793002/diff/120001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/cfi_unittest.cc:34: #if !defined(NDEBUG) On 2017/02/10 22:38:47, Will Harris wrote: > Consider just commenting out the entire test, instead of making it pass on debug > builds? Done. I'm fine either way.
The CQ bit was checked by pennymac@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2679793002/#ps140001 (title: "Code review fixes, part 2.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)
The CQ bit was checked by pennymac@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487105693787230, "parent_rev": "f7f8986f02c2f2acb3c3c9dbd21d606aaef189e0", "commit_rev": "8c67aad851ba03a83144557abfb2d4b3d182095e"}
Message was sent while issue was closed.
Description was changed from ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. TEST=New test CFGSupportTests.MsIndirectFailure in sbox_integration_tests suite. R=wfh BUG=584575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [Windows CFG Test] Added unittest for CFG enabling on process. Test ensures our build system enables CFG for a process when asked. Ensures that an MS system DLL in the process space is protected, even if none of Chromium code is compiled with CFG. TEST=New test CFGSupportTests.MsIndirectFailure in sbox_integration_tests suite. R=wfh BUG=584575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2679793002 Cr-Commit-Position: refs/heads/master@{#450498} Committed: https://chromium.googlesource.com/chromium/src/+/8c67aad851ba03a83144557abfb2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/8c67aad851ba03a83144557abfb2... |