Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(653)

Issue 1225183003: CreateThread interception, to use CreateRemoteThread (Closed)

Created:
5 years, 5 months ago by liamjm (20p)
Modified:
4 years, 10 months ago
CC:
chromium-reviews, rickyz+watch_chromium.org, wfh+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Intercept CreateThread inside renderer and proxy out to CreateRemoteThread in browser. summary: * Create new interception * Create new IPC (IPC_CREATETHREAD_TAG) * new tests in process_policy_test.cc * Interception is installed, but passes call through by default. Proxy is only done for tests, when ALPC ports are closed. Note: the IPC uses VOIDPTR_TYPE for a size_t param. These are not guaranteed to be the same (http://stackoverflow.com/questions/1464174/size-t-vs-intptr-t). Should we create a size_t IPC type? BUG=464430 Committed: https://crrev.com/80958824392b9ea4ec1932f02e2bd98b814591ad Cr-Commit-Position: refs/heads/master@{#374510}

Patch Set 1 #

Patch Set 2 : Bug fixes from rickyz #

Patch Set 3 : add thread_attributes param #

Patch Set 4 : remove changes to Visual Studio solution file #

Patch Set 5 : fix missing variable from cleanup #

Total comments: 12

Patch Set 6 : adds checks to new thread to ensure it is running as expected #

Patch Set 7 : load GetThreadId dynamically #

Total comments: 6

Patch Set 8 : Add some extra checks to test #

Patch Set 9 : sync to head #

Total comments: 15

Patch Set 10 : tweaks from review #

Total comments: 5

Patch Set 11 : Sync to head #

Patch Set 12 : using static_assert #

Patch Set 13 : remove thread_attributes from IPC call, use nullptr #

Total comments: 15

Patch Set 14 : some more tweaks, from XP code. #

Total comments: 8

Patch Set 15 : small tweaks #

Total comments: 2

Patch Set 16 : sync to head #

Patch Set 17 : Remove unneeded policy stuff, add method to policy_base to disconnect csrss #

Total comments: 2

Patch Set 18 : default to csrss not being disconnected #

Total comments: 6

Patch Set 19 : latest tweaks from wfh review #

Patch Set 20 : fix up casts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+326 lines, -23 lines) Patch
M sandbox/win/src/interceptors.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/src/interceptors_64.h View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
M sandbox/win/src/interceptors_64.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +14 lines, -0 lines 0 comments Download
M sandbox/win/src/ipc_tags.h View 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/src/policy_broker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -1 line 0 comments Download
M sandbox/win/src/policy_broker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +11 lines, -3 lines 0 comments Download
M sandbox/win/src/process_policy_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +103 lines, -2 lines 0 comments Download
M sandbox/win/src/process_thread_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +7 lines, -0 lines 0 comments Download
M sandbox/win/src/process_thread_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +31 lines, -0 lines 0 comments Download
M sandbox/win/src/process_thread_interception.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +12 lines, -2 lines 0 comments Download
M sandbox/win/src/process_thread_interception.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +78 lines, -1 line 0 comments Download
M sandbox/win/src/process_thread_policy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +10 lines, -0 lines 0 comments Download
M sandbox/win/src/process_thread_policy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +22 lines, -0 lines 0 comments Download
M sandbox/win/src/sandbox_policy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +11 lines, -7 lines 0 comments Download
M sandbox/win/src/sandbox_policy_base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +2 lines, -0 lines 0 comments Download
M sandbox/win/src/sandbox_policy_base.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +9 lines, -1 line 0 comments Download
M sandbox/win/src/top_level_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/tests/common/controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -6 lines 0 comments Download

Messages

Total messages: 51 (14 generated)
liamjm (20p)
5 years, 3 months ago (2015-09-01 18:52:32 UTC) #2
Will Harris
https://codereview.chromium.org/1225183003/diff/80001/sandbox/win/src/process_policy_test.cc File sandbox/win/src/process_policy_test.cc (right): https://codereview.chromium.org/1225183003/diff/80001/sandbox/win/src/process_policy_test.cc#newcode264 sandbox/win/src/process_policy_test.cc:264: return 0; I wonder if this should signal an ...
5 years, 3 months ago (2015-09-04 02:41:02 UTC) #3
liamjm (20p)
Thanks. Added checks to new thread to ensure it is running as expected. https://codereview.chromium.org/1225183003/diff/80001/sandbox/win/src/process_policy_test.cc File ...
5 years, 3 months ago (2015-09-04 21:30:39 UTC) #4
Will Harris
https://codereview.chromium.org/1225183003/diff/120001/sandbox/win/src/process_policy_test.cc File sandbox/win/src/process_policy_test.cc (right): https://codereview.chromium.org/1225183003/diff/120001/sandbox/win/src/process_policy_test.cc#newcode318 sandbox/win/src/process_policy_test.cc:318: if (WaitForSingleObject(event, INFINITE) != WAIT_OBJECT_0) { nit: consider adding ...
5 years, 3 months ago (2015-09-17 23:08:00 UTC) #5
liamjm (20p)
On 2015/09/17 23:08:00, Will Harris (OOO until 19 Oct) wrote: > https://codereview.chromium.org/1225183003/diff/120001/sandbox/win/src/process_policy_test.cc > File sandbox/win/src/process_policy_test.cc ...
5 years, 2 months ago (2015-10-19 19:45:41 UTC) #6
liamjm (20p)
On 2015/09/17 23:08:00, Will Harris (OOO until 19 Oct) wrote: > https://codereview.chromium.org/1225183003/diff/120001/sandbox/win/src/process_policy_test.cc > File sandbox/win/src/process_policy_test.cc ...
5 years, 2 months ago (2015-10-19 19:45:43 UTC) #7
liamjm (20p)
https://codereview.chromium.org/1225183003/diff/120001/sandbox/win/src/process_policy_test.cc File sandbox/win/src/process_policy_test.cc (right): https://codereview.chromium.org/1225183003/diff/120001/sandbox/win/src/process_policy_test.cc#newcode318 sandbox/win/src/process_policy_test.cc:318: if (WaitForSingleObject(event, INFINITE) != WAIT_OBJECT_0) { On 2015/09/17 23:08:00, ...
5 years, 2 months ago (2015-10-19 20:17:43 UTC) #8
Will Harris
https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc File sandbox/win/src/process_policy_test.cc (right): https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc#newcode312 sandbox/win/src/process_policy_test.cc:312: return SBOX_TEST_FAILED; if the interception returns 0 thread_id on ...
5 years ago (2015-12-03 06:41:51 UTC) #9
liamjm (20p)
thanks. https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc File sandbox/win/src/process_policy_test.cc (right): https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc#newcode312 sandbox/win/src/process_policy_test.cc:312: return SBOX_TEST_FAILED; On 2015/12/03 06:41:50, Will Harris wrote: ...
5 years ago (2015-12-03 21:53:30 UTC) #10
Will Harris
almost there! https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc File sandbox/win/src/process_policy_test.cc (right): https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc#newcode468 sandbox/win/src/process_policy_test.cc:468: TargetPolicy::PROCESS_MIN_EXEC, On 2015/12/03 21:53:29, liamjm (20p) wrote: ...
5 years ago (2015-12-03 23:58:14 UTC) #11
rickyz (no longer on Chrome)
https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc File sandbox/win/src/process_policy_test.cc (right): https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc#newcode468 sandbox/win/src/process_policy_test.cc:468: TargetPolicy::PROCESS_MIN_EXEC, On 2015/12/03 at 23:58:13, Will Harris (OOO until ...
5 years ago (2015-12-07 10:37:54 UTC) #13
rickyz (no longer on Chrome)
On 2015/12/07 at 10:37:54, rickyz wrote: > https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc > File sandbox/win/src/process_policy_test.cc (right): > > https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_policy_test.cc#newcode468 ...
4 years, 11 months ago (2016-01-16 07:24:42 UTC) #14
liamjm (20p)
Small fixes for a couple of comments. https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_thread_policy.cc File sandbox/win/src/process_thread_policy.cc (right): https://codereview.chromium.org/1225183003/diff/160001/sandbox/win/src/process_thread_policy.cc#newcode265 sandbox/win/src/process_thread_policy.cc:265: DCHECK(true); On ...
4 years, 10 months ago (2016-02-01 23:36:39 UTC) #15
liamjm (20p)
https://codereview.chromium.org/1225183003/diff/180001/sandbox/win/src/process_thread_interception.cc File sandbox/win/src/process_thread_interception.cc (right): https://codereview.chromium.org/1225183003/diff/180001/sandbox/win/src/process_thread_interception.cc#newcode458 sandbox/win/src/process_thread_interception.cc:458: reinterpret_cast<LPVOID>(start_address), On 2015/12/03 23:58:14, Will Harris wrote: > can ...
4 years, 10 months ago (2016-02-02 00:23:39 UTC) #16
Will Harris
adding forshaw for questions especially when creating this thread in target what are correct flags ...
4 years, 10 months ago (2016-02-02 05:45:33 UTC) #18
forshaw
https://codereview.chromium.org/1225183003/diff/240001/sandbox/win/src/process_thread_policy.cc File sandbox/win/src/process_thread_policy.cc (right): https://codereview.chromium.org/1225183003/diff/240001/sandbox/win/src/process_thread_policy.cc#newcode244 sandbox/win/src/process_thread_policy.cc:244: DWORD ProcessPolicy::CreateThreadAction( Is this only going to be used ...
4 years, 10 months ago (2016-02-02 11:11:49 UTC) #19
liamjm (20p)
updates from reviews by wfh@ and foreshaw@ https://codereview.chromium.org/1225183003/diff/240001/sandbox/win/src/process_thread_interception.cc File sandbox/win/src/process_thread_interception.cc (right): https://codereview.chromium.org/1225183003/diff/240001/sandbox/win/src/process_thread_interception.cc#newcode411 sandbox/win/src/process_thread_interception.cc:411: // GetThreadId ...
4 years, 10 months ago (2016-02-02 20:43:46 UTC) #21
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/1225183003/diff/260001/sandbox/win/src/process_thread_interception.cc File sandbox/win/src/process_thread_interception.cc (right): https://codereview.chromium.org/1225183003/diff/260001/sandbox/win/src/process_thread_interception.cc#newcode7 sandbox/win/src/process_thread_interception.cc:7: #include "base/win/windows_version.h" wrong include order? https://codereview.chromium.org/1225183003/diff/260001/sandbox/win/src/process_thread_interception.cc#newcode423 sandbox/win/src/process_thread_interception.cc:423: hThread = ...
4 years, 10 months ago (2016-02-04 01:40:52 UTC) #22
jschuh
For larger context, this morning I pointed liamjm@ to OpenThread() as a better model for ...
4 years, 10 months ago (2016-02-04 04:49:51 UTC) #23
liamjm (20p)
https://codereview.chromium.org/1225183003/diff/260001/sandbox/win/src/process_thread_policy.cc File sandbox/win/src/process_thread_policy.cc (right): https://codereview.chromium.org/1225183003/diff/260001/sandbox/win/src/process_thread_policy.cc#newcode258 sandbox/win/src/process_thread_policy.cc:258: CreateRemoteThread(client_info.process, nullptr, stack_size, On 2016/02/04 01:40:52, cpu wrote: > ...
4 years, 10 months ago (2016-02-04 21:56:38 UTC) #24
cpu_(ooo_6.6-7.5)
I guess my fundamental question is that would it be a better design if we ...
4 years, 10 months ago (2016-02-04 22:50:07 UTC) #25
jschuh
On 2016/02/04 22:50:07, cpu wrote: > I guess my fundamental question is that would it ...
4 years, 10 months ago (2016-02-04 23:09:21 UTC) #26
liamjm (20p)
Changes to CreateThread to make it more like OpenThread. Removed policy stuff from CreateThread. Added ...
4 years, 10 months ago (2016-02-05 19:22:44 UTC) #27
cpu_(ooo_6.6-7.5)
looking great, one final question: https://codereview.chromium.org/1225183003/diff/320001/sandbox/win/src/sandbox_policy_base.cc File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/1225183003/diff/320001/sandbox/win/src/sandbox_policy_base.cc#newcode136 sandbox/win/src/sandbox_policy_base.cc:136: is_csrss_connected_(false), true ?
4 years, 10 months ago (2016-02-05 20:49:37 UTC) #28
liamjm (20p)
https://codereview.chromium.org/1225183003/diff/320001/sandbox/win/src/sandbox_policy_base.cc File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/1225183003/diff/320001/sandbox/win/src/sandbox_policy_base.cc#newcode136 sandbox/win/src/sandbox_policy_base.cc:136: is_csrss_connected_(false), On 2016/02/05 20:49:37, cpu wrote: > true ? ...
4 years, 10 months ago (2016-02-05 21:53:08 UTC) #29
cpu_(ooo_6.6-7.5)
lgtm I might have reduced the concurrent IPC slots to 4 at some point, if ...
4 years, 10 months ago (2016-02-05 23:20:40 UTC) #30
Will Harris
lgtm with a few nits https://codereview.chromium.org/1225183003/diff/340001/sandbox/win/src/sandbox_policy.h File sandbox/win/src/sandbox_policy.h (right): https://codereview.chromium.org/1225183003/diff/340001/sandbox/win/src/sandbox_policy.h#newcode212 sandbox/win/src/sandbox_policy.h:212: // Disconnect the target ...
4 years, 10 months ago (2016-02-05 23:27:01 UTC) #31
liamjm (20p)
thanks https://codereview.chromium.org/1225183003/diff/340001/sandbox/win/src/sandbox_policy.h File sandbox/win/src/sandbox_policy.h (right): https://codereview.chromium.org/1225183003/diff/340001/sandbox/win/src/sandbox_policy.h#newcode212 sandbox/win/src/sandbox_policy.h:212: // Disconnect the target from CSRSS. On 2016/02/05 ...
4 years, 10 months ago (2016-02-05 23:51:22 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225183003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1225183003/360001
4 years, 10 months ago (2016-02-05 23:53:19 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/164699)
4 years, 10 months ago (2016-02-06 01:47:33 UTC) #37
Will Harris
e:\b\build\slave\win\build\src\sandbox\win\src\process_policy_test.cc(287): error C2220: warning treated as error - no 'object' file generated e:\b\build\slave\win\build\src\sandbox\win\src\process_policy_test.cc(287): warning C4311: ...
4 years, 10 months ago (2016-02-06 04:17:02 UTC) #38
liamjm (20p)
On 2016/02/06 04:17:02, Will Harris (ooo until 25 Feb) wrote: > e:\b\build\slave\win\build\src\sandbox\win\src\process_policy_test.cc(287): > error C2220: ...
4 years, 10 months ago (2016-02-09 22:07:34 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225183003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1225183003/380001
4 years, 10 months ago (2016-02-09 22:08:42 UTC) #44
commit-bot: I haz the power
Committed patchset #20 (id:380001)
4 years, 10 months ago (2016-02-09 22:45:37 UTC) #46
commit-bot: I haz the power
Patchset 20 (id:??) landed as https://crrev.com/80958824392b9ea4ec1932f02e2bd98b814591ad Cr-Commit-Position: refs/heads/master@{#374510}
4 years, 10 months ago (2016-02-09 22:47:41 UTC) #48
Nico
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang64/builds/3448/steps/compile/logs/stdio FAILED: ninja -t msvc -e environment.x64 -- "..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo /showIncludes /FC @obj\sandbox\win\src\sbox_integration_tests.process_policy_test.obj.rsp /c ...
4 years, 10 months ago (2016-02-10 02:33:59 UTC) #50
liamjm (20p)
4 years, 10 months ago (2016-02-11 05:02:43 UTC) #51
Message was sent while issue was closed.
On 2016/02/10 02:33:59, Nico wrote:
>
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang64/builds/3448/s...
> 
> FAILED: ninja -t msvc -e environment.x64 --
> "..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo
> /showIncludes /FC
> @obj\sandbox\win\src\sbox_integration_tests.process_policy_test.obj.rsp /c
> ..\..\sandbox\win\src\process_policy_test.cc
> /Foobj\sandbox\win\src\sbox_integration_tests.process_policy_test.obj
> /Fdobj\sandbox\sbox_integration_tests.cc.pdb 
> ..\..\sandbox\win\src\process_policy_test.cc(317,27) :  error: cast to 'void
*'
> from smaller integer type 'DWORD' (aka 'unsigned long')
> [-Werror,-Wint-to-void-pointer-cast]
>                           (LPVOID)pid, 0, &thread_id);
>                           ^
> ..\..\sandbox\win\src\process_policy_test.cc(504,31) :  error: cast to 'void
*'
> from smaller integer type 'DWORD' (aka 'unsigned long')
> [-Werror,-Wint-to-void-pointer-cast]
>                               (LPVOID)pid, 0, &thread_id);
>                               ^
> 
> 
> I'll send you a fix. (Passing a DWORD _in_ a void* seems a bit questionable,
but
> hey, windows code ¯\(°_o)/¯)

ACK.
Thanks for the fix.

Powered by Google App Engine
This is Rietveld 408576698