Bug 14853

Summary: Incorrect implementation of ArrayImpl's equality operator
Product: WebKit Reporter: rick
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor    
Priority: P3    
Version: 312.x   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch that fixes the described bug (includes ChangeLog differences) timothy: review+

Description rick 2007-08-01 12:04:02 PDT
The ArrayImpl class (as defined in WebCore/platform/ArrayImpl.{h,cpp}) has a typo in it's equality operator which could lead to an incorrect equality test and also a buffer overrun (and therefore a potential crash).

The fix is a trivial change to one line of code. The erroneous code contains the test:
	d->itemSize == d->itemSize
which should be:
	d->itemSize == a.d->itemSize



In practice, the bug will probably never be seen as the only (current) use of the ArrayImpl class is by DeprecatedArray (and it's subclass DeprecatedCString). This means that the value of itemSize will always be sizeof(char) (i.e. 1) for both objects being compared and the equality test will work correctly.



On the other hand, this bug appears in the exact same form in much older versions of ArrayImpl. In particular, the existing code has been copied from WebCore/kwq/KWQArrayImpl.cpp (which was copied from WebCore/kwq/KWQArrayImpl.mm).

I didn't dig too deep but previous versions of WebKit may have additional dependencies on these older classes and cases where itemSize is different. This bug may therefore be more serious in older versions of WebKit.
Comment 1 rick 2007-08-01 12:06:36 PDT
Created attachment 15785 [details]
Patch that fixes the described bug (includes ChangeLog differences)
Comment 2 David Kilzer (:ddkilzer) 2007-08-02 06:17:15 PDT
(In reply to comment #1)
> Created an attachment (id=15785) [edit]
> Patch that fixes the described bug (includes ChangeLog differences)

This really needs a test case.

Comment 3 rick 2007-08-02 08:04:10 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=15785) [edit]
> > Patch that fixes the described bug (includes ChangeLog differences)
> 
> This really needs a test case.

Where can I add such a test case? The problem is that the bug wouldn't show itself with current higher level functionality (i.e. a layout test or a JavaScriptCore test). I couldn't find anything about adding a lower level C++ test program. For example, something like:

#include "ArrayImpl.h"

int
main()
{
	WebCore::ArrayImpl a(1, 1);
	WebCore::ArrayImpl b(2, 1);
	
	// return 1 on error
	return a == b ? 1 : 0;
}
Comment 4 David Kilzer (:ddkilzer) 2007-08-02 09:58:08 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > This really needs a test case.
> 
> Where can I add such a test case? The problem is that the bug wouldn't show
> itself with current higher level functionality (i.e. a layout test or a
> JavaScriptCore test). I couldn't find anything about adding a lower level C++
> test program. For example, something like:

Sorry, by "test case" I meant a layout test (in HTML/JavaScript).  Since that's not possible, it's okay not to have one.  :)

Comment 5 Mark Rowe (bdash) 2007-08-29 10:45:11 PDT
Landed in r25297.