Bug 14853 - Incorrect implementation of ArrayImpl's equality operator
: Incorrect implementation of ArrayImpl's equality operator
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 312.x
: All All
: P3 Minor
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2007-08-01 12:04 PST by
Modified: 2007-08-29 10:45 PST (History)


Attachments
Patch that fixes the described bug (includes ChangeLog differences) (1.10 KB, patch)
2007-08-01 12:06 PST, rick@writhe.org.uk
timothy: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2007-08-01 12:04:02 PST
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 From 2007-08-01 12:06:36 PST -------
Created an attachment (id=15785) [details]
Patch that fixes the described bug (includes ChangeLog differences)
------- Comment #2 From 2007-08-02 06:17:15 PST -------
(In reply to comment #1)
> Created an attachment (id=15785) [edit] [details]
> Patch that fixes the described bug (includes ChangeLog differences)

This really needs a test case.
------- Comment #3 From 2007-08-02 08:04:10 PST -------
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=15785) [edit] [details]
> > 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 From 2007-08-02 09:58:08 PST -------
(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 From 2007-08-29 10:45:11 PST -------
Landed in r25297.