Bug 22134 - -[WebHistoryItem dictionaryRepresentation] accesses past the end of a vector
Summary: -[WebHistoryItem dictionaryRepresentation] accesses past the end of a vector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-07 17:00 PST by Aaron Golden
Modified: 2008-11-11 14:14 PST (History)
1 user (show)

See Also:


Attachments
This patch prevents the bad access. (674 bytes, patch)
2008-11-07 17:05 PST, Aaron Golden
no flags Details | Formatted Diff | Diff
Same patch as before, but with a bit more whitespace and a ChangeLog... (1.12 KB, patch)
2008-11-07 17:11 PST, Aaron Golden
no flags Details | Formatted Diff | Diff
New patch (1.61 KB, patch)
2008-11-10 12:42 PST, Aaron Golden
ggaren: review+
Details | Formatted Diff | Diff

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