Bug 26462

Summary:

Out of bounds read / occasional crash (no security consequence) in comment parsing

Product:

WebKit

Reporter:

Chris Evans <scarybeasts>

Component:

Page Loading

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

URL:

https://cevans-app.appspot.com/static/webkitcharsetoobread.html

Attachments:

Description	Flags
Fixes out-of-bounds reads.	abarth: review-
Fix out-of-bounds reads.	eric: review+

Chris Evans

Reported 2009-06-16 16:01:36 PDT

Patch based on the following crash triggering several times a day: Thread 1 *CRASHED* (EXCEPTION_ACCESS_VIOLATION @0x01a0b000) 0x01e97f41 [chrome.dll - textresourcedecoder.cpp:621] WebCore::TextResourceDecoder::checkForHeadCharset(char const *,unsigned int,bool &) 0x01e9832f [chrome.dll - textresourcedecoder.cpp:800] WebCore::TextResourceDecoder::decode(char const *,unsigned int) 0x01e42e05 [chrome.dll - frameloader.cpp:1813] WebCore::FrameLoader::addData(char const *,int) 0x02149041 [chrome.dll - webframe_impl.cc:1608] WebFrameImpl::DidReceiveData(WebCore::DocumentLoader *,char const *,int) Something has crashed due to crossing a page boundary. Upon inspection of the code at this location, skipComment() clearly reads past the end of the buffer. I have verified this in the debugger. No reliable test case is available because of the non-deterministic nature of reading out-of-bounds. In fact I have never observed a crash -- but it is clearly happening many times a day in the crash logs. Fix is easy / obvious -- patch upload to follow.

Attachments
Fixes out-of-bounds reads. (1015 bytes, patch) 2009-06-16 16:02 PDT, Chris Evans	abarth: review-	Details Formatted Diff Diff
Fix out-of-bounds reads. (1.89 KB, patch) 2009-06-18 18:49 PDT, Chris Evans	eric: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Chris Evans

Comment 1 2009-06-16 16:02:52 PDT

Created attachment 31390 [details] Fixes out-of-bounds reads.

Adam Barth

Comment 2 2009-06-16 16:09:44 PDT

Comment on attachment 31390 [details] Fixes out-of-bounds reads. Thanks for the patch, but we'll need to include a ChangeLog before landing this.

Darin Adler

Comment 3 2009-06-16 17:12:13 PDT

Also, WebKit coding style has no braces around a single line if body.

Chris Evans

Comment 4 2009-06-18 18:49:51 PDT

Created attachment 31528 [details] Fix out-of-bounds reads.

Chris Evans

Comment 5 2009-06-18 18:50:56 PDT

Request land of patch. Changes since last version: - Adhere to coding style (thanks Darin)! - Add ChangeLog entry. - Includes justification of why I can't reasonably add a test.

Eric Seidel (no email)

Comment 6 2009-06-18 18:57:19 PDT

Comment on attachment 31528 [details] Fix out-of-bounds reads. Tabs in the ChangeLog. Otherwise this looks great. Will have to be landed manually, my script won't be able to fix the tabs (it's not that smart).

Brent Fulgham

Comment 7 2009-06-19 12:48:41 PDT

Assigned for landing.

Brent Fulgham

Comment 8 2009-06-19 12:55:27 PDT

Corrected tabs and landed in http://trac.webkit.org/changeset/44865.

David Kilzer (:ddkilzer)

Comment 9 2009-06-22 17:51:08 PDT

PLEASE add bug numbers to ChangeLog entries in the future.

Chris Evans

Comment 10 2009-06-22 17:55:43 PDT

You got it. Sorry :)

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