Bug 14853 – Incorrect implementation of ArrayImpl's equality operator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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.

Created attachment 15785 [details]
Patch that fixes the described bug (includes ChangeLog differences)

[David Kilzer (:ddkilzer)]

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

(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;
}

[David Kilzer (:ddkilzer)]

(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.  :)

[Mark Rowe (bdash)]

Landed in r25297.