Bug 22134

Summary: -[WebHistoryItem dictionaryRepresentation] accesses past the end of a vector
Product: WebKit Reporter: Aaron Golden <agolden>
Component: HistoryAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
This patch prevents the bad access.
none
Same patch as before, but with a bit more whitespace and a ChangeLog...
none
New patch ggaren: review+

Description Aaron Golden 2008-11-07 17:00:25 PST
The for loop to iterate over the children vector in -[WebHistoryItem dictionaryRepresentation] starts out with i = children.size(), so if we ever hit that loop we're going to attempt an access past the end of the children vector.  It should be i = children.size()-1.
Comment 1 Aaron Golden 2008-11-07 17:05:48 PST
Created attachment 24979 [details]
This patch prevents the bad access.
Comment 2 Aaron Golden 2008-11-07 17:11:58 PST
Created attachment 24980 [details]
Same patch as before, but with a bit more whitespace and a ChangeLog...
Comment 3 Darin Adler 2008-11-09 11:50:56 PST
Comment on attachment 24980 [details]
Same patch as before, but with a bit more whitespace and a ChangeLog...

This change is clearly correct, but we normally require regression tests for bug fixes. Were you able to reproduce a problem? I'd love to see a test and not just the fix.

r=me
Comment 4 Aaron Golden 2008-11-10 12:42:30 PST
Created attachment 25025 [details]
New patch

It turns out that -[WebHistoryItem initWithDictionaryRepresentation] has the same problem as -[WebHistoryItem dictionaryRepresentation] so fixing that as well and updating the ChangeLog.
Comment 5 David Kilzer (:ddkilzer) 2008-11-10 14:42:42 PST
Comment on attachment 24980 [details]
Same patch as before, but with a bit more whitespace and a ChangeLog...

Clearing review flag on unlanded patch.
Comment 6 Aaron Golden 2008-11-10 15:06:05 PST
(In reply to comment #3)
> (From update of attachment 24980 [details] [review])
> This change is clearly correct, but we normally require regression tests for
> bug fixes. Were you able to reproduce a problem? I'd love to see a test and not
> just the fix.
> 
> r=me
> 

I think that the affected API is not currently used in the Safari browser, so I will not be able to provide a layout test.  In theory I could write a new application designed to demonstrate the problem, but in this case I doubt that it's necessary.
Comment 7 Geoffrey Garen 2008-11-11 09:24:43 PST
Comment on attachment 25025 [details]
New patch

r=me
Comment 8 David Kilzer (:ddkilzer) 2008-11-11 14:14:59 PST
$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/History/WebHistoryItem.mm
Committed r38315