Bug 26462 - Out of bounds read / occasional crash (no security consequence) in comment parsing
Summary: Out of bounds read / occasional crash (no security consequence) in comment pa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: https://cevans-app.appspot.com/static...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-16 16:01 PDT by Chris Evans
Modified: 2009-06-22 17:55 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Evans 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.
Comment 1 Chris Evans 2009-06-16 16:02:52 PDT
Created attachment 31390 [details]
Fixes out-of-bounds reads.
Comment 2 Adam Barth 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.
Comment 3 Darin Adler 2009-06-16 17:12:13 PDT
Also, WebKit coding style has no braces around a single line if body.
Comment 4 Chris Evans 2009-06-18 18:49:51 PDT
Created attachment 31528 [details]
Fix out-of-bounds reads.
Comment 5 Chris Evans 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.
Comment 6 Eric Seidel (no email) 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).
Comment 7 Brent Fulgham 2009-06-19 12:48:41 PDT
Assigned for landing.
Comment 8 Brent Fulgham 2009-06-19 12:55:27 PDT
Corrected tabs and landed in http://trac.webkit.org/changeset/44865.
Comment 9 David Kilzer (:ddkilzer) 2009-06-22 17:51:08 PDT
PLEASE add bug numbers to ChangeLog entries in the future.
Comment 10 Chris Evans 2009-06-22 17:55:43 PDT
You got it. Sorry :)