Bug 16888 - -webkit-border-image crash/invalid free
Summary: -webkit-border-image crash/invalid free
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P1 Major
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2008-01-15 22:17 PST by Michael Goddard
Modified: 2008-01-24 06:20 PST (History)
1 user (show)

See Also:


Attachments
Testcase and patch (2.75 KB, patch)
2008-01-15 22:17 PST, Michael Goddard
sam: review-
Details | Formatted Diff | Diff
Fix memory corruption - just store Values as member vars, don't allocate them (5.59 KB, patch)
2008-01-16 16:13 PST, Michael Goddard
no flags Details | Formatted Diff | Diff
Use naked pointers rather than OwnPtrs since the source pointers are in the middle of an array (3.37 KB, patch)
2008-01-17 15:54 PST, Michael Goddard
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Goddard 2008-01-15 22:17:15 PST
There's an error in the CSSParser when parsing the width components of -webkit-border-image.  A pointer to the middle of an array is stored in an OwnPtr and gets freed.  Can cause crashes/memory corruption.

Testcase/patch to be attached.
Comment 1 Michael Goddard 2008-01-15 22:17:43 PST
Created attachment 18465 [details]
Testcase and patch
Comment 2 Michael Goddard 2008-01-15 22:18:53 PST
The test case style for the <div> could probably be removed.
Comment 3 Sam Weinig 2008-01-15 23:07:35 PST
Comment on attachment 18465 [details]
Testcase and patch

There are some tabs in the patch and I don't understand the ChangeLog entirely.  Where is the OwnPtr that you are storing to?
Comment 4 Dave Hyatt 2008-01-16 11:26:30 PST
Comment on attachment 18465 [details]
Testcase and patch

I don't quite understand this patch either.  I don't see why a Value would have to be copied.
Comment 5 Michael Goddard 2008-01-16 16:13:14 PST
Created attachment 18485 [details]
Fix memory corruption - just store Values as member vars, don't allocate them

In the original code, m_borderTop etc were OwnPtr<Value>s, and so we needed to give them a valid pointer (hence the allocation in the previous patch, rather than the middle of an array).  Since the BorderImageParseContext is stack allocated anyway, just make it slightly larger to hold actual Values and copy them in.  This needs an extra variable to track which Values are valid.
Comment 6 Michael Goddard 2008-01-17 15:36:35 PST
Another option would be to just store pointers to the middle of the callers ValueList, since hopefully the lifetime is longer than the BorderImageParseContext.
Comment 7 Michael Goddard 2008-01-17 15:54:47 PST
Created attachment 18516 [details]
Use naked pointers rather than OwnPtrs since the source pointers are in the middle of an array

Simpler than previous patch and avoids any extra allocations.
Comment 8 David Kilzer (:ddkilzer) 2008-01-19 02:07:36 PST
Loading the test case, the following is printed to the console for times per page load on a debug build of WebKit r29623:

Safari(28864,0xa000ed88) malloc: ***  Deallocation of a pointer not malloced: 0xc199030; This could be a double free(), or free() called with the middle of an allocated block; Try setting environment variable MallocHelp to see tools to help debug
Comment 9 David Kilzer (:ddkilzer) 2008-01-19 02:09:41 PST
<rdar://problem/5696235>
Comment 10 Darin Adler 2008-01-22 08:37:01 PST
Comment on attachment 18516 [details]
Use naked pointers rather than OwnPtrs since the source pointers are in the middle of an array

r=me
Comment 11 Simon Hausmann 2008-01-24 06:20:08 PST
Landed in r29764