Bug 11924 - WebCore crash on 64-bit code when loading a web page with a table tag
Summary: WebCore crash on 64-bit code when loading a web page with a table tag
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 420+
Hardware: PC Linux
: P2 Blocker
Assignee: Nobody
Depends on:
Reported: 2006-12-22 00:03 PST by Martin Reddy
Modified: 2007-01-13 11:06 PST (History)
1 user (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Reddy 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:

  <!-- any non-empty table tag will crash WebKit, e.g. -->

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;

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.


Comment 1 David Kilzer (:ddkilzer) 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:


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.
Comment 2 Darin Adler 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.
Comment 3 Martin Reddy 2007-01-12 17:59:02 PST
Created attachment 12407 [details]
Patch with fix for Bug 11924
Comment 4 Martin Reddy 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.) 
Comment 5 Darin Adler 2007-01-13 07:13:11 PST
Comment on attachment 12407 [details]
Patch with fix for Bug 11924

Looks good. r=me
Comment 6 Sam Weinig 2007-01-13 11:06:39 PST
Landed in r18834!