Bug 16888

Summary:

-webkit-border-image crash/invalid free

Product:

WebKit

Reporter:

Michael Goddard <michael.goddard>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Major

CC:

hausmann

Priority:

Keywords:

HasReduction, InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

All

Attachments:

Description	Flags
Testcase and patch	sam: review-
Fix memory corruption - just store Values as member vars, don't allocate them	none
Use naked pointers rather than OwnPtrs since the source pointers are in the middle of an array	darin: review+

Michael Goddard

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Michael Goddard

Comment 1 2008-01-15 22:17:43 PST

Created attachment 18465 [details] Testcase and patch

Michael Goddard

Comment 2 2008-01-15 22:18:53 PST

The test case style for the <div> could probably be removed.

Sam Weinig

Comment 3 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?

Dave Hyatt

Comment 4 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.

Michael Goddard

Comment 5 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.

Michael Goddard

Comment 6 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.

Michael Goddard

Comment 7 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.

David Kilzer (:ddkilzer)

Comment 8 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

David Kilzer (:ddkilzer)

Comment 9 2008-01-19 02:09:41 PST

<rdar://problem/5696235>

Darin Adler

Comment 10 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

Simon Hausmann

Comment 11 2008-01-24 06:20:08 PST

Landed in r29764

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