RESOLVED FIXED 83751
[Chromium] The size of glyphStorage should be passed to substituteWithVerticalGlyphs() 
https://bugs.webkit.org/show_bug.cgi?id=83751
Summary [Chromium] The size of glyphStorage should be passed to substituteWithVertica...
Kenichi Ishibashi
Reported 2012-04-12 00:32:48 PDT
GlyphPage::fill() in GlyphPageTreeNodeSkia.cpp calls substituteWithVerticalGlyphs() with wrong buffer length. The last argument should be the length of |glyphStorage|.
Attachments
Patch (1.55 KB, patch)
2012-04-12 00:38 PDT, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Patch (1.65 KB, patch)
2012-04-12 01:10 PDT, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kenichi Ishibashi
Comment 1 2012-04-12 00:38:04 PDT
Created attachment 136838 [details] Patch
Kenichi Ishibashi
Comment 2 2012-04-12 00:41:33 PDT
Kent-san, Tony, Could you take a look?
Kent Tamura
Comment 3 2012-04-12 00:49:14 PDT
Comment on attachment 136838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136838&action=review > Source/WebCore/ChangeLog:3 > + [Chromium] Fix OOB in substituteWithVerticalGlyphs() "OOB" sounds like a security bug. > Source/WebCore/ChangeLog:11 > + No new tests. No behavior change. Really? This patch looks a fix of a problem.
Kenichi Ishibashi
Comment 4 2012-04-12 01:01:16 PDT
Comment on attachment 136838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136838&action=review >> Source/WebCore/ChangeLog:3 >> + [Chromium] Fix OOB in substituteWithVerticalGlyphs() > > "OOB" sounds like a security bug. I agree. I'll change the title >> Source/WebCore/ChangeLog:11 >> + No new tests. No behavior change. > > Really? This patch looks a fix of a problem. You are right. However, I can't figure out how to test the fix. We can see the crash when we use ASAN-enabled chromium, but without ASAN, we can't see any difference at layout test level. Do you think changing the comment to describe why this patch lacks tests is enough?
Kent Tamura
Comment 5 2012-04-12 01:06:50 PDT
(In reply to comment #4) > Do you think changing the comment to describe why this patch lacks tests is enough? I think so.
Kenichi Ishibashi
Comment 6 2012-04-12 01:10:09 PDT
Created attachment 136844 [details] Patch
Kenichi Ishibashi
Comment 7 2012-04-12 01:10:45 PDT
(In reply to comment #5) > (In reply to comment #4) > > Do you think changing the comment to describe why this patch lacks tests is enough? > > I think so. Thanks. I've updated the patch.
Kent Tamura
Comment 8 2012-04-12 01:11:45 PDT
Comment on attachment 136844 [details] Patch ok
WebKit Review Bot
Comment 9 2012-04-12 01:49:21 PDT
Comment on attachment 136844 [details] Patch Clearing flags on attachment: 136844 Committed r113951: <http://trac.webkit.org/changeset/113951>
WebKit Review Bot
Comment 10 2012-04-12 01:49:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.