Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2020863002:
Guard against invalid glyph shaping results (Closed)
|
Created:
4 years, 6 months ago by pdr. Modified:
4 years, 6 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGuard against invalid glyph shaping results
HarfBuzz can fail to shape all glyphs and will return a shape result
shorter than the text length along with debug warnings:
[ERROR:HarfBuzzShaper.cpp(375)] HarfBuzz returned empty glyph buffer after shaping.
[ERROR:HarfBuzzShaper.cpp(672)] Shape result extraction failed.
This patch fixes an SVG crash on the U+180E Mongolian vowel separator
by ensuring CachingWordShaper::individualCharacterRanges returns a
vector as long as the text run length. A DCHECK and test have been added
to protect against this crash in the future.
BUG=613915
Committed: https://crrev.com/433ceaeabee98073e81d552dee947aa6983efd53
Cr-Commit-Position: refs/heads/master@{#396668}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address reviewer comments #
Messages
Total messages: 21 (9 generated)
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020863002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2020863002/1
Description was changed from ========== Guard against invalid glyph shaping results HarfBuzz can fail to shape all glyphs and will return a shape result shorter than the text length along with debug warnings: [ERROR:HarfBuzzShaper.cpp(375)] HarfBuzz returned empty glyph buffer after shaping. [ERROR:HarfBuzzShaper.cpp(672)] Shape result extraction failed. This patch fixes an SVG crash on the U+180E Mongolian vowel separator by ensuring CachingWordShaper::individualCharacterRanges returns a vector as long as the text run length. A DCHECK and test has been added to protect against this in the future. BUG=613915 ========== to ========== Guard against invalid glyph shaping results HarfBuzz can fail to shape all glyphs and will return a shape result shorter than the text length along with debug warnings: [ERROR:HarfBuzzShaper.cpp(375)] HarfBuzz returned empty glyph buffer after shaping. [ERROR:HarfBuzzShaper.cpp(672)] Shape result extraction failed. This patch fixes an SVG crash on the U+180E Mongolian vowel separator by ensuring CachingWordShaper::individualCharacterRanges returns a vector as long as the text run length. A DCHECK and test have been added to protect against this crash in the future. BUG=613915 ==========
pdr@chromium.org changed reviewers: + eae@chromium.org, fs@opera.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ a few points to ponder. I'm find with landing this as-is and fixing up things later, if deemed appropriate. https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/text/invalid-glyph-crash.html (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/text/invalid-glyph-crash.html:9: document.querySelector('text').innerHTML = 'PASS᠎'; This doesn't look like something the parser would "object" to (turn into replacement char or so). Does it still reproduce if it's just specified as part of the markup directly? https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/Font.cpp:771: DCHECK(ranges.size() == static_cast<unsigned>(run.length())); Nit: DCHECK_EQ? https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/shaping/CachingWordShaper.cpp (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShaper.cpp:130: while (ranges.size() < static_cast<unsigned>(run.length())) So this will do all the padding "at the end", while I guess there could be multiple failures in a set of shape results? Or will Harfbuzz fail on the first so that there'll only ever be one failure? (I.e, ought the character offset be tracked down in the ShapeResultBuffer methods so that padding can be applied in a way so that the "fixups" line up with the errors.) Anyway, this seems unlikely enough that applying a band-aid is hopefully fine, and potential follow-on errors higher up will be benign. (Also, should this be CharacterRange(ranges.last().end, ranges.last().end)? doesn't matter either ATM because we only use width() IIRC.)
https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/text/invalid-glyph-crash.html (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/text/invalid-glyph-crash.html:9: document.querySelector('text').innerHTML = 'PASS᠎'; On 2016/05/28 at 14:08:31, fs wrote: > This doesn't look like something the parser would "object" to (turn into replacement char or so). Does it still reproduce if it's just specified as part of the markup directly? Yeah, this still works / noworks without script. Switched to putting PASS and the vowel separator in the DOM instead of in script. https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/Font.cpp:771: DCHECK(ranges.size() == static_cast<unsigned>(run.length())); On 2016/05/28 at 14:08:31, fs wrote: > Nit: DCHECK_EQ? Done (also have a followup to remove those unruly static cats in https://codereview.chromium.org/2018253002 but lets land this first). https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/shaping/CachingWordShaper.cpp (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShaper.cpp:130: while (ranges.size() < static_cast<unsigned>(run.length())) On 2016/05/28 at 14:08:31, fs wrote: > So this will do all the padding "at the end", while I guess there could be multiple failures in a set of shape results? Or will Harfbuzz fail on the first so that there'll only ever be one failure? (I.e, ought the character offset be tracked down in the ShapeResultBuffer methods so that padding can be applied in a way so that the "fixups" line up with the errors.) > > Anyway, this seems unlikely enough that applying a band-aid is hopefully fine, and potential follow-on errors higher up will be benign. > > (Also, should this be CharacterRange(ranges.last().end, ranges.last().end)? doesn't matter either ATM because we only use width() IIRC.) Great questions. Ultimately I think this should be fixed down in HarfBuzzShaper.cpp instead of doing too much work to fixup the results up here. I think the code added in this patch will still be a useful guard against bad shaping results even after that is fixed though, since this is the first code that directly uses shape results which exposed this bug. I've filed https://crbug.com/615661 for the HarfBuzz fix. Does that sound reasonable to you?
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020863002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2020863002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/Font.cpp:771: DCHECK(ranges.size() == static_cast<unsigned>(run.length())); On 2016/05/28 at 22:58:15, pdr. wrote: > On 2016/05/28 at 14:08:31, fs wrote: > > Nit: DCHECK_EQ? > > Done (also have a followup to remove those unruly static cats in https://codereview.chromium.org/2018253002 but lets land this first). Yeah, no one likes static cats... https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/shaping/CachingWordShaper.cpp (right): https://codereview.chromium.org/2020863002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/shaping/CachingWordShaper.cpp:130: while (ranges.size() < static_cast<unsigned>(run.length())) On 2016/05/28 at 22:58:16, pdr. wrote: > On 2016/05/28 at 14:08:31, fs wrote: > > So this will do all the padding "at the end", while I guess there could be multiple failures in a set of shape results? Or will Harfbuzz fail on the first so that there'll only ever be one failure? (I.e, ought the character offset be tracked down in the ShapeResultBuffer methods so that padding can be applied in a way so that the "fixups" line up with the errors.) > > > > Anyway, this seems unlikely enough that applying a band-aid is hopefully fine, and potential follow-on errors higher up will be benign. > > > > (Also, should this be CharacterRange(ranges.last().end, ranges.last().end)? doesn't matter either ATM because we only use width() IIRC.) > > Great questions. > > Ultimately I think this should be fixed down in HarfBuzzShaper.cpp instead of doing too much work to fixup the results up here. I think the code added in this patch will still be a useful guard against bad shaping results even after that is fixed though, since this is the first code that directly uses shape results which exposed this bug. I've filed https://crbug.com/615661 for the HarfBuzz fix. > > Does that sound reasonable to you? Yes, I do prefer less pairs of belts and suspenders. SGTM.
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/2020863002/#ps20001 (title: "Address reviewer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020863002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2020863002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Guard against invalid glyph shaping results HarfBuzz can fail to shape all glyphs and will return a shape result shorter than the text length along with debug warnings: [ERROR:HarfBuzzShaper.cpp(375)] HarfBuzz returned empty glyph buffer after shaping. [ERROR:HarfBuzzShaper.cpp(672)] Shape result extraction failed. This patch fixes an SVG crash on the U+180E Mongolian vowel separator by ensuring CachingWordShaper::individualCharacterRanges returns a vector as long as the text run length. A DCHECK and test have been added to protect against this crash in the future. BUG=613915 ========== to ========== Guard against invalid glyph shaping results HarfBuzz can fail to shape all glyphs and will return a shape result shorter than the text length along with debug warnings: [ERROR:HarfBuzzShaper.cpp(375)] HarfBuzz returned empty glyph buffer after shaping. [ERROR:HarfBuzzShaper.cpp(672)] Shape result extraction failed. This patch fixes an SVG crash on the U+180E Mongolian vowel separator by ensuring CachingWordShaper::individualCharacterRanges returns a vector as long as the text run length. A DCHECK and test have been added to protect against this crash in the future. BUG=613915 Committed: https://crrev.com/433ceaeabee98073e81d552dee947aa6983efd53 Cr-Commit-Position: refs/heads/master@{#396668} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/433ceaeabee98073e81d552dee947aa6983efd53 Cr-Commit-Position: refs/heads/master@{#396668}
Message was sent while issue was closed.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
