|
|
DescriptionModifed platform bitmap to support win32k lockdown.
This patch detects if the current process is under win32k lockdown
and instead of using the GDI route it maps the shared section directly.
This is done for supporting the introduction of win32k lockdown for
PPAPI processes. To minimize the chances of disruption when not under
win32k lockdown the code path is kept the same.
BUG=523278
Committed: https://crrev.com/e201ebab608f5ff0a12b3e1de96585bcafb28517
Cr-Commit-Position: refs/heads/master@{#350828}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Minor stylistic changes for review. #
Total comments: 1
Patch Set 3 : Made policy check static and added comment. #
Total comments: 1
Patch Set 4 : Used common Win32k check. #Patch Set 5 : Fixes from review. #Messages
Total messages: 29 (9 generated)
forshaw@chromium.org changed reviewers: + palmer@chromium.org, wfh@chromium.org
Please take a look.
wfh@chromium.org changed reviewers: + scottmg@chromium.org
+scottmg knows this code. I had a feeling there was a non DC based platform device somewhere already? https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... File skia/ext/bitmap_platform_device_win.cc (right): https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:37: return false; can you cache the result of this function with a static, maybe? https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:150: nit: line
scottmg@chromium.org changed reviewers: + ananta@chromium.org
Sorry, I'm not familiar with a non-DC device. Maybe you were thinking of +ananta (I think worked on TransportDIB stuff for lockdown). https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... File skia/ext/bitmap_platform_device_win.cc (right): https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:22: GetProcessMitigationPolicyType GetProcessMitigationPolicyFunc = GetProcessMitigationPolicyFunc should be get_process_mitigation_policy[_func] https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:23: reinterpret_cast<GetProcessMitigationPolicyType>(::GetProcAddress( remove unnecessary :: qualifiers (or add them everywhere, but it looks like most don't have them in this file (e.g. MapViewOfFile, GetCurrentProcess) https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:168: HBITMAP hbitmap = NULL; NULL -> nullptr, and elsewhere
On 2015/09/04 02:25:50, Will Harris wrote: > +scottmg knows this code. I had a feeling there was a non DC based platform > device somewhere already? There is sort of, for example https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p.... But that (I believe) is part of the renderer host. Currently both PDFium and Flash end up calling into chrome_child!skia::BitmapPlatformDevice::Create when they receive a ViewChange message form the PPAPI host which ultimately hits this location. Seemed simpler and catch more edge cases to change here as you can't continue if you reach this code with GDI disabled unless you just try and do a raw mapping.
PTAL. https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... File skia/ext/bitmap_platform_device_win.cc (right): https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:22: GetProcessMitigationPolicyType GetProcessMitigationPolicyFunc = On 2015/09/04 02:46:19, scottmg wrote: > GetProcessMitigationPolicyFunc should be get_process_mitigation_policy[_func] Done. https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:23: reinterpret_cast<GetProcessMitigationPolicyType>(::GetProcAddress( On 2015/09/04 02:46:19, scottmg wrote: > remove unnecessary :: qualifiers (or add them everywhere, but it looks like most > don't have them in this file (e.g. MapViewOfFile, GetCurrentProcess) Done. https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:37: return false; On 2015/09/04 02:25:50, Will Harris wrote: > can you cache the result of this function with a static, maybe? Will cache with static, don't think it needs to be thread safe. https://codereview.chromium.org/1321913002/diff/1/skia/ext/bitmap_platform_de... skia/ext/bitmap_platform_device_win.cc:168: HBITMAP hbitmap = NULL; On 2015/09/04 02:46:19, scottmg wrote: > NULL -> nullptr, and elsewhere Was just being consistent with the rest of the file which used NULL. Don't want to unnecessarily change all uses of NULL unless there's a policy to move all modified code to using nullptr or live with mixed usage in a single file.
wfh@chromium.org changed reviewers: + reed@chromium.org
I think reed should look at this, as he's familiar with this code. https://codereview.chromium.org/1321913002/diff/20001/skia/ext/bitmap_platfor... File skia/ext/bitmap_platform_device_win.cc (right): https://codereview.chromium.org/1321913002/diff/20001/skia/ext/bitmap_platfor... skia/ext/bitmap_platform_device_win.cc:38: return false; I suppose I more meant make the return value of the entire function static i.e. cached, but I don't know how long the call to GetProcessMitigationPolicy takes.
reed@google.com changed reviewers: + reed@google.com
This is pretty far above my head. I don't understand (certainly not from reading the new code) what LockDown means and/or why it affects some but not other methods in that file. Perhaps this is just my ignorance, but a comment block or some other explanation at the start of the file might be helpful for future maintainers, etc. Is there any way to test this (e.g. unittests or otherwise)?
On 2015/09/04 17:23:00, reed1 wrote: > This is pretty far above my head. I don't understand (certainly not from reading > the new code) what LockDown means and/or why it affects some but not other > methods in that file. Perhaps this is just my ignorance, but a comment block or > some other explanation at the start of the file might be helpful for future > maintainers, etc. > > Is there any way to test this (e.g. unittests or otherwise)? I've added a comment in the latest version which hopefully explains the point of the change. As for unit tests, it's theoretically possible but tricky to do without sandbox support. I could add a way of faking the policy value and check if you felt that was really worth it. As there isn't any existing unit tests for this code it would be starting from scratch.
Updated the code, PTAL.
https://codereview.chromium.org/1321913002/diff/40001/skia/ext/bitmap_platfor... File skia/ext/bitmap_platform_device_win.cc (right): https://codereview.chromium.org/1321913002/diff/40001/skia/ext/bitmap_platfor... skia/ext/bitmap_platform_device_win.cc:69: static base::LazyInstance<LazyIsWin32kLockdownEnabled> win32k_lockdown; I think this needs a LAZY_INSTANCE_INITIALIZER
PTAL, hopefully all fixed now.
The Windows code lgtm, so we should land this if the skia people are happy for this fallback path to land.
ananta do you have views on this CL? otherwise, if we can get skia owners to l-g-t-m that would unblock forshaw. Thanks
LGTM, FWIW.
Patchset #6 (id:100001) has been deleted
Patchset #4 (id:60001) has been deleted
reed@ PTAL. I've changed the patch slightly to make it more explicit what the code is doing. I've added the comment above the actual affected area of the code and removed all discussion of win32k lockdown, instead describing it in terms of whether the GDI APIs are or are not available. In default operation this shouldn't change the code in any way, it only affects operation if GDI is disabled through some mechanism so regression potential should be zero. I believe this is the most appropriate place to put these changes as no code above this in the call stack (such as TransportDIB or ppapi::proxy::PlatformImageData knows anything about the internal guts of the platform device at least with respect to GDI use. This means that the platform canvas stuff could easily change from under them and they shouldn't notice the difference.
lgtm
On 2015/09/25 13:24:32, reed1 wrote: > lgtm Thanks very much for your review. It's very much appreciated.
The CQ bit was checked by forshaw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/1321913002/#ps120001 (title: "Fixes from review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321913002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321913002/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e201ebab608f5ff0a12b3e1de96585bcafb28517 Cr-Commit-Position: refs/heads/master@{#350828} |