Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1720643002:
[Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. (Closed)
|
Created:
4 years, 10 months ago by penny Modified:
4 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, rickyz+watch_chromium.org, forshaw Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment.
Enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation
in child processes only when Direct Write is enabled (not for GDI).
This CL follows https://codereview.chromium.org/1666753002.
R=wfh@chromium.org,jschuh@chromium.org
BUG=504006, 586291
Committed: https://crrev.com/247218e258a2491967b63d5dddb63a1e90b9dc42
Cr-Commit-Position: refs/heads/master@{#377110}
Patch Set 1 #Patch Set 2 : <sigh> #
Total comments: 5
Patch Set 3 : Code review changes, part 1. Only enable for DirectWrite. #
Total comments: 2
Patch Set 4 : Code review fixes, part 2. #Messages
Total messages: 15 (6 generated)
Description was changed from ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Do not enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes when Direct Write is enabled. This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006 ========== to ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Do not enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes when Direct Write is enabled. This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006 ==========
Description was changed from ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Do not enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes when Direct Write is enabled. This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006 ========== to ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Do not enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes when Direct Write is enabled. This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006,586291 ==========
Hello hello. Looks like MITIGATION_NONSYSTEM_FONT_DISABLE was causing problems for a few Win10 users with Direct Write enabled (on canary/dev). Not surprising. Here's a small patch to not enable the mitigation when DW is on. Thanks!
This is why I'm sure we're not testing the GDI path at all. https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... content/common/sandbox_win.cc:706: Strike MITIGATION_NONSYSTEM_FONT_DISABLE in the default flags above and add it in conditionally like this: if (gfx::win::ShouldUseDirectWrite()) mitigations |= sandbox::MITIGATION_NONSYSTEM_FONT_DISABLE; https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... content/common/sandbox_win.cc:743: // when Direct Write is enabled. I think this is backward. Don't you want to enable the mitigation only when DW is enabled?
I had it backwards. Need to enable this mitigation only for Direct Write (not the other way around). It's GDI that might have to load font files on the fly. Thanks for the review Justin! https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... content/common/sandbox_win.cc:706: On 2016/02/22 22:30:25, jschuh (very slow) wrote: > Strike MITIGATION_NONSYSTEM_FONT_DISABLE in the default flags above and add it > in conditionally like this: > > if (gfx::win::ShouldUseDirectWrite()) > mitigations |= sandbox::MITIGATION_NONSYSTEM_FONT_DISABLE; Done. I'm fairly sure it needs to be wrapped in "#if !defined(NACL_WIN64)". Let me know if not. https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... content/common/sandbox_win.cc:743: // when Direct Write is enabled. On 2016/02/22 22:30:24, jschuh (very slow) wrote: > I think this is backward. Don't you want to enable the mitigation only when DW > is enabled? > Done.
Description was changed from ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Do not enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes when Direct Write is enabled. This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006,586291 ========== to ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes only when Direct Write is enabled (not for GDI). This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006,586291 ==========
drive by https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/1720643002/diff/20001/content/common/sandbox_... content/common/sandbox_win.cc:706: On 2016/02/23 05:30:37, penny wrote: > On 2016/02/22 22:30:25, jschuh (very slow) wrote: > > Strike MITIGATION_NONSYSTEM_FONT_DISABLE in the default flags above and add it > > in conditionally like this: > > > > if (gfx::win::ShouldUseDirectWrite()) > > mitigations |= sandbox::MITIGATION_NONSYSTEM_FONT_DISABLE; > > Done. I'm fairly sure it needs to be wrapped in "#if !defined(NACL_WIN64)". > Let me know if not. yes it does - since direct_write.cc isn't compiled with nacl64. You'd get linker errors otherwise. https://codereview.chromium.org/1720643002/diff/40001/content/common/sandbox_... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/1720643002/diff/40001/content/common/sandbox_... content/common/sandbox_win.cc:709: #endif for the NACL_WIN64 case, I think we can always enable andbox::MITIGATION_NONSYSTEM_FONT_DISABLE
Made sure the mitigation is always set, unless GDI. https://codereview.chromium.org/1720643002/diff/40001/content/common/sandbox_... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/1720643002/diff/40001/content/common/sandbox_... content/common/sandbox_win.cc:709: #endif On 2016/02/23 05:46:02, Will Harris (ooo until 25 Feb) wrote: > for the NACL_WIN64 case, I think we can always enable > andbox::MITIGATION_NONSYSTEM_FONT_DISABLE Done.
lgtm!
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720643002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720643002/60001
Message was sent while issue was closed.
Description was changed from ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes only when Direct Write is enabled (not for GDI). This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006,586291 ========== to ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes only when Direct Write is enabled (not for GDI). This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006,586291 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes only when Direct Write is enabled (not for GDI). This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006,586291 ========== to ========== [Win10 sandbox mitigations] MITIGATION_NONSYSTEM_FONT_DISABLE adjustment. Enable MITIGATION_NONSYSTEM_FONT_DISABLE mitigation in child processes only when Direct Write is enabled (not for GDI). This CL follows https://codereview.chromium.org/1666753002. R=wfh@chromium.org,jschuh@chromium.org BUG=504006,586291 Committed: https://crrev.com/247218e258a2491967b63d5dddb63a1e90b9dc42 Cr-Commit-Position: refs/heads/master@{#377110} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/247218e258a2491967b63d5dddb63a1e90b9dc42 Cr-Commit-Position: refs/heads/master@{#377110} |
