| 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: | P2 | ||||||||
| Version: | 420+ | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Attachments: |
|
||||||||
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. 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. Created attachment 12407 [details] Patch with fix for Bug 11924 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 on attachment 12407 [details] Patch with fix for Bug 11924 Looks good. r=me |
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> <!-- any non-empty table tag will crash WebKit, e.g. --> <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.