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

Issue 919893002: Replace handles that the handle closer closes with dummy Events. (Closed)

Created:
5 years, 10 months ago by Will Harris
Modified:
5 years, 10 months ago
CC:
chromium-reviews, kalyank, sadrul, 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

Replace handles that the handle closer closes with dummy Events. This is because on Windows, we close some handles opened by Windows DLLs to prevent them from leaking into sandboxed processes, and this can cause INVALID_HANDLE_EXCEPTION to be raised if "bad handle detection" gflag is enabled, as it currently is by default on Windows 10. Only replace File and Event handles at the moment, as File handles are the ones causing the issues on Windows 10. BUG=452613 TEST=sbox_integration_tests TEST=Run Chrome on Windows 10 with crash reporting enabled, and check there are no crashes listed in chrome://crashes Committed: https://crrev.com/b21f85ae8442e9fd831b70427c1c1919d8116af6 Cr-Commit-Position: refs/heads/master@{#317016}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : will put sandbox change in other CL #

Patch Set 4 : only change win #

Total comments: 4

Patch Set 5 : address nits #

Patch Set 6 : consume invalid handle exceptions on shutdown of sandboxed process #

Patch Set 7 : Stuff closed handles with an empty Event #

Patch Set 8 : only stuff events and file handles #

Total comments: 15

Patch Set 9 : address comments. add test. #

Patch Set 10 : add max count #

Patch Set 11 : remaining nits #

Total comments: 1

Patch Set 12 : add comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -3 lines) Patch
M sandbox/win/src/handle_closer_agent.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +8 lines, -2 lines 0 comments Download
M sandbox/win/src/handle_closer_agent.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +46 lines, -0 lines 0 comments Download
M sandbox/win/src/handle_closer_test.cc View 1 2 3 4 5 6 7 8 5 chunks +104 lines, -1 line 0 comments Download

Messages

Total messages: 48 (14 generated)
Will Harris
5 years, 10 months ago (2015-02-12 03:58:23 UTC) #2
Will Harris
+rsesek for views on linux/mac changes.
5 years, 10 months ago (2015-02-12 17:52:38 UTC) #4
Robert Sesek
On Mac and Linux, this will be adding uncalled code, since those platforms don't use ...
5 years, 10 months ago (2015-02-12 18:59:28 UTC) #5
Will Harris
On 2015/02/12 18:59:28, Robert Sesek wrote: > On Mac and Linux, this will be adding ...
5 years, 10 months ago (2015-02-12 19:08:49 UTC) #6
Robert Sesek
LGTM https://codereview.chromium.org/919893002/diff/60001/components/crash/app/breakpad_win.h File components/crash/app/breakpad_win.h (right): https://codereview.chromium.org/919893002/diff/60001/components/crash/app/breakpad_win.h#newcode20 components/crash/app/breakpad_win.h:20: // Shuts down Breakpad. "Should be called immediately ...
5 years, 10 months ago (2015-02-12 19:18:04 UTC) #7
Will Harris
Thanks for the review, Robert. https://codereview.chromium.org/919893002/diff/60001/components/crash/app/breakpad_win.h File components/crash/app/breakpad_win.h (right): https://codereview.chromium.org/919893002/diff/60001/components/crash/app/breakpad_win.h#newcode20 components/crash/app/breakpad_win.h:20: // Shuts down Breakpad. ...
5 years, 10 months ago (2015-02-12 20:38:50 UTC) #9
cpu_(ooo_6.6-7.5)
will is going to add more context in the bug but I need to have ...
5 years, 10 months ago (2015-02-13 00:17:11 UTC) #11
Will Harris
I commented on the bug. I think the solution of having a call into breakpad ...
5 years, 10 months ago (2015-02-13 01:45:48 UTC) #12
Will Harris
I retested patch set 5 and I am getting TERMINATION_STATUS_PROCESS_CRASHED from child sandboxed processes (as ...
5 years, 10 months ago (2015-02-14 00:31:07 UTC) #13
Will Harris
hmm vmware cut 'n' paste error - I meant ChildProcess.Crashed2 for this histogram for crashed ...
5 years, 10 months ago (2015-02-14 00:31:41 UTC) #14
Will Harris
PTAL - I have the LGTM on the Breakpad CL - https://breakpad.appspot.com/7794002/ now, but won't ...
5 years, 10 months ago (2015-02-15 22:58:41 UTC) #15
Sigurður Ásgeirsson
On 2015/02/15 22:58:41, Will Harris wrote: > PTAL - I have the LGTM on the ...
5 years, 10 months ago (2015-02-17 15:22:42 UTC) #16
Sigurður Ásgeirsson
Also the CL description needs updating.
5 years, 10 months ago (2015-02-17 15:23:30 UTC) #17
Sigurður Ásgeirsson
NVM, I guess the email thread retains an earlier description as subject. On Tue Feb ...
5 years, 10 months ago (2015-02-17 15:24:42 UTC) #18
Will Harris
Turns out, stuffing the handles with dummy Events works really well, and causes all EXCEPTION_INVALID_HANDLE ...
5 years, 10 months ago (2015-02-18 00:10:56 UTC) #21
Will Harris
PTAL. I've also only whitelisted handles of type File or Event for the handle stuffing.
5 years, 10 months ago (2015-02-18 01:14:17 UTC) #22
forshaw
> The only added > risk is that previously operations such as WaitForSingleObject would be ...
5 years, 10 months ago (2015-02-18 10:41:32 UTC) #23
forshaw
https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_closer_agent.cc File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_closer_agent.cc#newcode40 sandbox/win/src/handle_closer_agent.cc:40: HANDLE dummy = ::CreateEvent(NULL, FALSE, FALSE, NULL); Can we ...
5 years, 10 months ago (2015-02-18 10:50:10 UTC) #24
Sigurður Ásgeirsson
lgtm modulo a nit. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_closer_agent.cc File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_closer_agent.cc#newcode46 sandbox/win/src/handle_closer_agent.cc:46: DWORD options = DUPLICATE_SAME_ACCESS; looks ...
5 years, 10 months ago (2015-02-18 13:56:00 UTC) #25
cpu_(ooo_6.6-7.5)
looks good, I think it might desirable to set the security descriptor of the object ...
5 years, 10 months ago (2015-02-18 18:00:00 UTC) #26
cpu_(ooo_6.6-7.5)
oh one more, we need to limit the dup while loop to a reasonable number ...
5 years, 10 months ago (2015-02-18 18:01:00 UTC) #27
forshaw
> I think it might desirable to set the security descriptor of the object so ...
5 years, 10 months ago (2015-02-18 18:04:26 UTC) #28
Will Harris
On 2015/02/18 18:04:26, forshaw wrote: > Events are another one of those objects where the ...
5 years, 10 months ago (2015-02-18 18:08:59 UTC) #29
Sigurður Ásgeirsson
https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_closer_agent.cc File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_closer_agent.cc#newcode55 sandbox/win/src/handle_closer_agent.cc:55: break; come to think of it, another check you ...
5 years, 10 months ago (2015-02-18 19:02:47 UTC) #30
Will Harris
PTAL - I think I've addressed everyone's comment, and I added a test that shows ...
5 years, 10 months ago (2015-02-18 22:11:29 UTC) #33
cpu_(ooo_6.6-7.5)
lgtm https://codereview.chromium.org/919893002/diff/220001/sandbox/win/src/handle_closer_agent.cc File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/220001/sandbox/win/src/handle_closer_agent.cc#newcode47 sandbox/win/src/handle_closer_agent.cc:47: write a comment here about the technique including ...
5 years, 10 months ago (2015-02-19 00:10:03 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919893002/240001
5 years, 10 months ago (2015-02-19 03:13:26 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/27982)
5 years, 10 months ago (2015-02-19 04:50:27 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919893002/240001
5 years, 10 months ago (2015-02-19 05:30:20 UTC) #41
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/28031)
5 years, 10 months ago (2015-02-19 06:59:53 UTC) #43
Will Harris
this is a flake caused by 2f7a8e80b83aa581aa6de86117a692093d16c217 which has already been reverted. I suppose I ...
5 years, 10 months ago (2015-02-19 07:05:18 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919893002/240001
5 years, 10 months ago (2015-02-19 08:17:36 UTC) #46
commit-bot: I haz the power
Committed patchset #12 (id:240001)
5 years, 10 months ago (2015-02-19 09:01:40 UTC) #47
commit-bot: I haz the power
5 years, 10 months ago (2015-02-19 09:02:16 UTC) #48
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/b21f85ae8442e9fd831b70427c1c1919d8116af6
Cr-Commit-Position: refs/heads/master@{#317016}

Powered by Google App Engine
This is Rietveld 408576698