Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1977833003:
Add a buildflag to use the handle verifier in a per module mode. (Closed)
|
Created:
4 years, 7 months ago by Sébastien Marchand Modified:
4 years, 6 months ago CC:
chromium-reviews, grt+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. |
DescriptionAdd a buildflag to use the handle verifier in a per module mode.
The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode..
Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192
Cr-Commit-Position: refs/heads/master@{#398361}
BUG=618205
Committed: https://crrev.com/82ef0204161f34381ab832a576c4789e1063e0ee
Cr-Commit-Position: refs/heads/master@{#398575}
Patch Set 1 #
Total comments: 2
Patch Set 2 : GN. #Patch Set 3 : GN. #
Total comments: 1
Patch Set 4 : Add a flag to use the HV in a single module mode. #Patch Set 5 : Add a flag to use the HV in a single module mode. #Patch Set 6 : Add a flag to use the HV in a single module mode. #
Total comments: 2
Patch Set 7 : Fix. #
Total comments: 2
Patch Set 8 : Switch to a normal deps. #
Total comments: 2
Patch Set 9 : comment. #Patch Set 10 : Fix the GN config. #
Messages
Total messages: 52 (18 generated)
Description was changed from ========== g# Enter a description of the change. Add a buildflag to disable the handle verifier. BUG= ========== to ========== Add a buildflag to disable the handle verifier. ==========
sebmarchand@chromium.org changed reviewers: + wfh@chromium.org
Hey, what do you think of this approach to have a way to disable the handle verifier code ? It's still a WIP, the gyp changes are not complete and GN is still missing, I just wanted a first opinion on this.
Description was changed from ========== Add a buildflag to disable the handle verifier. ========== to ========== WIP: Add a buildflag to disable the handle verifier. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to disable it in the projects that don't need it. ==========
sebmarchand@chromium.org changed reviewers: + chrisha@chromium.org
+chrisha@, here's a WIP for the handle verifier build flag.
Looks like a sane way to accomplish this to me, but I don't know much about the build traits system. https://codereview.chromium.org/1977833003/diff/1/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1977833003/diff/1/base/base.gyp#newcode1037 base/base.gyp:1037: # Since this generates a file, it most only be referenced in the target must* (could fix this where it was copied from too)
So the Q about whether we want to expose a way to totally disable the verifier - the handle verifier can also operate in single module mode - this is here: https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_ha... where it does not call cross-module but instead just creates one per module. I'd be interested if just using this code fixes the issues you are having with syzygy. Handle verifier in single module mode still will tell you if you do bad things like double-close a handle... which is worth keeping if it works in your code. https://codereview.chromium.org/1977833003/diff/40001/base/win/BUILD.gn File base/win/BUILD.gn (right): https://codereview.chromium.org/1977833003/diff/40001/base/win/BUILD.gn#newcode8 base/win/BUILD.gn:8: enable_handle_verifier = true nit: needs a comment here for the gn help.
Yeah, I don't mind keeping it if the overhead is low and if there's no potential compatibility issue. We ship SyzyAsan as an external DLL that get used by Chrome, they never use the same revision of base/ so we should try to avoid using anything that might cause some compatibility issue. Using one verifier per module seems safe enough. Chris, wdyt ? https://codereview.chromium.org/1977833003/diff/1/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1977833003/diff/1/base/base.gyp#newcode1037 base/base.gyp:1037: # Since this generates a file, it most only be referenced in the target On 2016/05/13 18:44:54, chrisha (slow) wrote: > must* > > (could fix this where it was copied from too) Done.
Chris: ping ? Should we keep the verifier or should we get rid of it ? (see Will's comment about this).
I'm fine with keeping it if it runs in a 'per module' mode. Whatever is easiest, IMO.
On 2016/05/24 17:20:28, chrisha (slow) wrote: > I'm fine with keeping it if it runs in a 'per module' mode. Whatever is easiest, > IMO. fine with that too, perhaps close this CL unless you are planning on working on it, and then raise a new one that allows per-module to be set?
I'm reworking this CL, I'll update the description and send a request for review soon-ish.
Description was changed from ========== WIP: Add a buildflag to disable the handle verifier. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to disable it in the projects that don't need it. ========== to ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. ==========
Ok, reworked this to use this flag on a per module mode, PTAL.
https://codereview.chromium.org/1977833003/diff/100001/base/win/scoped_handle... File base/win/scoped_handle_unittest.cc (right): https://codereview.chromium.org/1977833003/diff/100001/base/win/scoped_handle... base/win/scoped_handle_unittest.cc:52: #if BUILDFLAG(SINGLE_MODULE_MODE_HANDLE_VERIFIER) why do these tests now depend on this, they didn't before
Thanks, PTAnL. https://codereview.chromium.org/1977833003/diff/100001/base/win/scoped_handle... File base/win/scoped_handle_unittest.cc (right): https://codereview.chromium.org/1977833003/diff/100001/base/win/scoped_handle... base/win/scoped_handle_unittest.cc:52: #if BUILDFLAG(SINGLE_MODULE_MODE_HANDLE_VERIFIER) On 2016/06/01 16:46:40, Will Harris (slow thru 23 May) wrote: > why do these tests now depend on this, they didn't before Hum, I thought that there was a #ifdef COMPONENT_BUILD here but it looks like a mistake during the switch to SINGLE_MODULE_MODE_HANDLE_VERIFIER ... Sorry ! Fixed.
+thakis@ for owner approval.
sebmarchand@chromium.org changed reviewers: + thakis@chromium.org
+thakis for owner approval (for real this time)
https://codereview.chromium.org/1977833003/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1977833003/diff/120001/base/BUILD.gn#newcode1182 base/BUILD.gn:1182: public_deps += [ "//base/win:base_win_features" ] it looks like the new feature define is only used in a cc file and its test. if you add this to public_deps, then every file in every target depending on base (almost all of them) will get a new -D flag, while it's really only needed in two files. Maybe this should just be an ordinary dep in base and base_unittests?
Thanks for the quick review. https://codereview.chromium.org/1977833003/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1977833003/diff/120001/base/BUILD.gn#newcode1182 base/BUILD.gn:1182: public_deps += [ "//base/win:base_win_features" ] On 2016/06/03 14:41:16, Nico wrote: > it looks like the new feature define is only used in a cc file and its test. if > you add this to public_deps, then every file in every target depending on base > (almost all of them) will get a new -D flag, while it's really only needed in > two files. Maybe this should just be an ordinary dep in base and base_unittests? Oh, I thought that we were strongly encouraged to always use the new buildflag definition for any new build flag. I agree that in this case it make more sense to just add the define for these 2 targets, I'll do this.
no i mean use the buildflag pattern, but rather than use a public_dep, make it a regular dep in the two places that are needed (3 line diff change from what you have)
Patchset #8 (id:140001) has been deleted
Ok, PTAnL.
don't quite understand the features stuff, happy to leave that to nico to review, but the .cc files lgtm
thakis@ : ping ?
lgtm https://codereview.chromium.org/1977833003/diff/160001/base/win/BUILD.gn File base/win/BUILD.gn (right): https://codereview.chromium.org/1977833003/diff/160001/base/win/BUILD.gn#newc... base/win/BUILD.gn:14: single_module_mode_handle_verifier = true this means that if you explicitly set single_module_mode_handle_verifier to false but do a component build, you end up with it true, yes? is that what you want?
On 2016/06/07 16:48:18, Nico wrote: > lgtm > > https://codereview.chromium.org/1977833003/diff/160001/base/win/BUILD.gn > File base/win/BUILD.gn (right): > > https://codereview.chromium.org/1977833003/diff/160001/base/win/BUILD.gn#newc... > base/win/BUILD.gn:14: single_module_mode_handle_verifier = true > this means that if you explicitly set single_module_mode_handle_verifier to > false but do a component build, you end up with it true, yes? is that what you > want? I think this is fine. In a component build then SINGLE_MODULE_MODE_HANDLE_VERIFIER should always be forced enabled and override what single_module_mode_handle_verifier was set to.
https://codereview.chromium.org/1977833003/diff/160001/base/win/BUILD.gn File base/win/BUILD.gn (right): https://codereview.chromium.org/1977833003/diff/160001/base/win/BUILD.gn#newc... base/win/BUILD.gn:14: single_module_mode_handle_verifier = true On 2016/06/07 16:48:18, Nico wrote: > this means that if you explicitly set single_module_mode_handle_verifier to > false but do a component build, you end up with it true, yes? is that what you > want? Yes, I'll add a comment.
The CQ bit was checked by sebmarchand@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/1977833003/#ps180001 (title: "comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977833003/180001
Message was sent while issue was closed.
Description was changed from ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. ========== to ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. ========== to ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361}
Message was sent while issue was closed.
battre@chromium.org changed reviewers: + battre@chromium.org
Message was sent while issue was closed.
This fails on Win7 reliably: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20(dbg)(1)/bu... Will revert to see whether that fixes the problem. ScopedHandleTest.ActiveVerifierDoubleTracking (run #1): [ RUN ] ScopedHandleTest.ActiveVerifierDoubleTracking c:uild\slave\win_builder__dbg_uild\srcase\win\scoped_handle_unittest.cc(74): error: Death test: { base::win::ScopedHandle handle_holder2(handle); } Result: failed to die. Error msg: [ DEATH ] [5396:3224:0607/231233:18621043:WARNING:test_suite.cc(195)] Test launcher output path C:\Users\CHROME~2\AppData\Local\TempK2_29056 est_results.xml exists. Not adding test launcher result printer. [ DEATH ] [ FAILED ] ScopedHandleTest.ActiveVerifierDoubleTracking (140 ms) ScopedHandleTest.ActiveVerifierDoubleTracking (run #2): [ RUN ] ScopedHandleTest.ActiveVerifierDoubleTracking c:uild\slave\win_builder__dbg_uild\srcase\win\scoped_handle_unittest.cc(74): error: Death test: { base::win::ScopedHandle handle_holder2(handle); } Result: failed to die. Error msg: [ DEATH ] [328:4444:0607/231241:18629030:WARNING:test_suite.cc(195)] Test launcher output path C:\Users\CHROME~2\AppData\Local\TempK2_12446 est_results.xml exists. Not adding test launcher result printer. [ DEATH ] [ FAILED ] ScopedHandleTest.ActiveVerifierDoubleTracking (141 ms) ScopedHandleTest.ActiveVerifierDoubleTracking (run #3): [ RUN ] ScopedHandleTest.ActiveVerifierDoubleTracking c:uild\slave\win_builder__dbg_uild\srcase\win\scoped_handle_unittest.cc(74): error: Death test: { base::win::ScopedHandle handle_holder2(handle); } Result: failed to die. Error msg: [ DEATH ] [3584:2092:0607/231242:18630606:WARNING:test_suite.cc(195)] Test launcher output path C:\Users\CHROME~2\AppData\Local\TempK2_18644 est_results.xml exists. Not adding test launcher result printer. [ DEATH ] [ FAILED ] ScopedHandleTest.ActiveVerifierDoubleTracking (187 ms) ScopedHandleTest.ActiveVerifierDoubleTracking (run #4): [ RUN ] ScopedHandleTest.ActiveVerifierDoubleTracking c:uild\slave\win_builder__dbg_uild\srcase\win\scoped_handle_unittest.cc(74): error: Death test: { base::win::ScopedHandle handle_holder2(handle); } Result: failed to die. Error msg: [ DEATH ] [4324:5172:0607/231244:18632135:WARNING:test_suite.cc(195)] Test launcher output path C:\Users\CHROME~2\AppData\Local\TempK2_6961 est_results.xml exists. Not adding test launcher result printer. [ DEATH ] [ FAILED ] ScopedHandleTest.ActiveVerifierDoubleTracking (140 ms)
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:180001) has been created in https://codereview.chromium.org/2047013003/ by battre@chromium.org. The reason for reverting is: ScopedHandleTest.* tests fail to die on try bots. BUG=618205.
Message was sent while issue was closed.
There was a missing '$' in base/win/BUILD.gn :(, PTAnL.
Message was sent while issue was closed.
Description was changed from ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} ========== to ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} ==========
Description was changed from ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} ========== to ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} BUG=618205 ==========
still lgtm 1.) For obvious one-byte fixes like this, I think you can just reland and don't need another review 2.) For fixes like this, it makes things much easier if you upload the cl unmodified right after rebasing, and then the fix in a separate patchset. then it's easier to find the fixed part
Thanks, I'll do this next time.
The CQ bit was checked by sebmarchand@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/1977833003/#ps200001 (title: "Fix the GN config.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977833003/200001
Message was sent while issue was closed.
Description was changed from ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} BUG=618205 ========== to ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} BUG=618205 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} BUG=618205 ========== to ========== Add a buildflag to use the handle verifier in a per module mode. The HandleVerifier prevent you from using any ScopedHandle during a module initialization (i.e. if a DLL use one in DllMain this will call back into its host executable that hasn't been initialized yet and things will derail). In this CL I'm adding a build flag to make it possible to use it in a single module mode.. Committed: https://crrev.com/a89708a5c1bfe778ed1615f0bc92f0e9ae2e2192 Cr-Commit-Position: refs/heads/master@{#398361} BUG=618205 Committed: https://crrev.com/82ef0204161f34381ab832a576c4789e1063e0ee Cr-Commit-Position: refs/heads/master@{#398575} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/82ef0204161f34381ab832a576c4789e1063e0ee Cr-Commit-Position: refs/heads/master@{#398575} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
