Bug 14853 - Incorrect implementation of ArrayImpl's equality operator
: Incorrect implementation of ArrayImpl's equality operator
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 312.x
: All All
: P3 Minor
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-01 12:04 PDT by rick
Modified: 2007-08-29 10:45 PDT (History)
0 users

See Also:


Attachments
Patch that fixes the described bug (includes ChangeLog differences) (1.10 KB, patch)
2007-08-01 12:06 PDT, rick
timothy: review+
Details | Formatted Diff | Diff

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