Bug 11924

Summary:

WebCore crash on 64-bit code when loading a web page with a table tag

Product:

WebKit

Reporter:

Martin Reddy <reddy>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Blocker

CC:

reddy

Priority:

Version:

420+

Hardware:

OS:

Linux

Attachments:

Description	Flags
Martin's patch in Comment #0	darin: review-
Patch with fix for Bug 11924	darin: review+

Martin Reddy

Reported 2006-12-22 00:03:53 PST

If you try to build WebKit as 64-bit code (-m64) then it will core dump whenever you try to load an HTML page that contains a <table> tag. For example, the following simple web page will crash WebKit when compiled 64-bit: <html><body>  <table><tr><td></td></tr></table> </body></html> The problem is in WebCore/rendering/RenderTableSection.cpp, in the ensureRows() method. The issue is this line: if (numRows > static_cast<int>(numeric_limits<size_t>::max() / sizeof(RowStruct))) In a 64-bit environment, size_t is a 64-bit value but int is 32-bit, so casting this very large number to a signed int will give you a negative number. This causes the vector to not be resized in the normal case. Instead, the result of the division should be kept as a size_t and numRows should be promoted to a size_t for the comparison (either implicitly or explicitly). Here's the result of 'svn diff' on a fix that I made for this bug in my tree: Index: RenderTableSection.cpp =================================================================== --- RenderTableSection.cpp (revision 18367) +++ RenderTableSection.cpp (working copy) @@ -149,7 +149,8 @@ int nRows = gridRows; if (numRows > nRows) { if (numRows > static_cast<int>(grid.size())) { - if (numRows > static_cast<int>(numeric_limits<size_t>::max() / sizeof(RowStruct))) + size_t maxSize = numeric_limits<size_t>::max() / sizeof(RowStruct); + if (static_cast<size_t>(numRows) > maxSize) return false; grid.resize(numRows); } This explicitly casts numRows to a size_t, which you don't have to do of course, but it does make it a bit more clear. Feel free to employ a better solution if you prefer though. I don't have write privileges to the WebKit tree, so I'm submitting this as a bug. I'd be happy to help out more though. Cheers, Martin.

Attachments
Martin's patch in Comment #0 (721 bytes, patch) 2006-12-22 01:36 PST, David Kilzer (:ddkilzer)	darin: review-	Details Formatted Diff Diff
Patch with fix for Bug 11924 (2.18 KB, patch) 2007-01-12 17:59 PST, Martin Reddy	darin: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

David Kilzer (:ddkilzer)

Comment 1 2006-12-22 01:36:28 PST

Created attachment 11961 [details] Martin's patch in Comment #0 Thanks for the patch, Martin! If you're going to be submitting a lot of fixes, I'd recommend reviewing this information on the webkit.org site: http://webkit.org/coding/contributing.html It doesn't mention it, but there is a prepare-ChangeLog script that helps you to create ChangeLog entries for your patches. Also note that both prepare-ChangeLog and svn-create-patch run much faster if you give them a list of files and directories for the patch you're working on.

Darin Adler

Comment 2 2006-12-22 17:39:16 PST

Comment on attachment 11961 [details] Martin's patch in Comment #0 This is a good fix for the numeric overflow. However, the crash indicates that the caller doesn't handle the case where ensureRows returns false. That should also be fixed. The callers that don't check are addChild and recalcCells. addCell does check. Setting review- on this because it needs change log and a test case and should also fix the callers that don't check the return value from ensureRows.

Martin Reddy

Comment 3 2007-01-12 17:59:02 PST

Created attachment 12407 [details] Patch with fix for Bug 11924

Martin Reddy

Comment 4 2007-01-12 18:06:22 PST

Good comments Darin. I've just attached a revised patch, made against the latest revision, that also add checks to the callers of ensureRows. And I've included a ChangeLog entry for the fix too (I left the reviewer as NOBODY - I don't know what your policy is on filling this in before accepting, so left it as-is). Does this patch look okay? In terms of adding a test, I'd be happy to do this, though I don't feel that this is perhaps necessary in this case. The reason being that any of the pre-existing WebCoreLayout/tables tests would crash on the current code under a 64-bit build. E.g., I'm guessing that anyone running WebKit built as a 64-bit app on Leopard would see this. (See the initial description for a trivial example of HTML content that crashed WebCore before my fix.)

Darin Adler

Comment 5 2007-01-13 07:13:11 PST

Comment on attachment 12407 [details] Patch with fix for Bug 11924 Looks good. r=me

Sam Weinig

Comment 6 2007-01-13 11:06:39 PST

Landed in r18834!

Note You need to log in before you can comment on or make changes to this bug.