Bug 139969

Summary:

[WinCairo] Crash when font data pointer is null.

Product:

WebKit

Reporter:

peavo

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alex.christensen, bfulgham, commit-queue, ossy, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

peavo

Reported 2014-12-27 13:01:48 PST

There is missing a null pointer check in the UniscribeController::shape() method. I have gotten several crashes here because the font data pointer was null.

Attachments
Patch (1.37 KB, patch) 2014-12-27 13:07 PST, peavo	no flags	Details Formatted Diff Diff
Patch (4.08 KB, patch) 2015-01-03 08:21 PST, peavo	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

peavo

Comment 1 2014-12-27 13:07:49 PST

Created attachment 243779 [details] Patch

Sam Weinig

Comment 2 2014-12-27 13:57:16 PST

Comment on attachment 243779 [details] Patch Test case?

Csaba Osztrogonác

Comment 3 2014-12-27 14:39:32 PST

(In reply to comment #2) > Comment on attachment 243779 [details] > Patch > > Test case? test for wincairo? :) It's impossible to run any layout test on wincairo. (N)RWT doesn't know wincairo port (bug124927) and there isn't ORWT long time ago ...

Sam Weinig

Comment 4 2014-12-27 15:14:59 PST

(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 243779 [details] > > Patch > > > > Test case? > > test for wincairo? :) > > It's impossible to run any layout test on wincairo. (N)RWT doesn't > know wincairo port (bug124927) and there isn't ORWT long time ago ... So, is it time to remove support for WinCairo? A port without any testing seems like a bad idea. Do you at least have a test case that crashes in a WinCairo mini browser?

Alex Christensen

Comment 5 2014-12-28 00:05:21 PST

I think we should not need both an assert and a null check. Asserts should be for things that should never be false. Sam, I have hardware for a WinCairo bot. See https://bugs.webkit.org/show_bug.cgi?id=139908 The few differences between AppleWin and WinCairo are tested manually and that has worked well for some time now.

Alex Christensen

Comment 6 2014-12-28 00:32:21 PST

And layout tests can be run with WinCairo, it just requires a bit of impersonating AppleWin right now.

peavo

Comment 7 2014-12-28 12:31:36 PST

(In reply to comment #2) > Comment on attachment 243779 [details] > Patch > > Test case? I will try to come up with a test case :)

peavo

Comment 8 2014-12-29 13:32:27 PST

Here is a somewhat reduced test case which crashes the browser every time: <html> <head> <style> span.texhtml{font-family:"Nimbus Roman No9 L","Times New Roman",Times,serif;font-size:118%;line-height:1} .texhtml{-webkit-font-feature-settings:"lnum","tnum";font-feature-settings:"lnum","tnum"} </style> </head> <body> <span class="texhtml">|<span class="Unicode">⟩</span></span> </body> </html>

Alex Christensen

Comment 9 2014-12-29 19:43:41 PST

Comment on attachment 243779 [details] Patch I assume this change fixes it so it doesn't crash. Then add the test to layout tests and upload a new patch.

peavo

Comment 10 2015-01-03 08:21:33 PST

Created attachment 243917 [details] Patch

peavo

Comment 11 2015-01-03 08:22:59 PST

(In reply to comment #9) > Comment on attachment 243779 [details] > Patch > > I assume this change fixes it so it doesn't crash. Then add the test to > layout tests and upload a new patch. Thanks for reviewing :) Updated patch.

WebKit Commit Bot

Comment 12 2015-01-05 07:05:48 PST

Comment on attachment 243917 [details] Patch Clearing flags on attachment: 243917 Committed r177909: <http://trac.webkit.org/changeset/177909>

WebKit Commit Bot

Comment 13 2015-01-05 07:05:53 PST

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.