654932 - Field names array in ArrayClass::generic_sortOn defeats reference counting

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Lars T Hansen]

The code is constructing a heap-allocated array that's an implicit subclass of GCObject, with each element of the type FieldName:

        struct FieldName
        {
            RCObject::GCMember<String> name;
            int options;
        };

(That could have been a GCInlineObject but that would not have changed the point of this bug.)

The names will correctly have their reference counts incremented upon being stored in the array, but because the array's destructor is never called (it's a GCObject) the names' reference counts will not be decremented when the array is GC'd.  As a consequence, any names participating can only ever be reclaimed by garbage collection.

This is one instance where a disciplined "GCArray" type would probably have helped us write correct code more easily.

Comment 1

[Lars T Hansen]

Indeed it would appear that the field name array should be allocated by means of VMPI_alloca.  There's some cleanup to do; ~ArraySort explicitly deletes the field names array, but there's really no need for that kind of song and dance so far as I can see.

Comment 2

[Lars T Hansen]

Attachment: Convert to VMPI_alloca

Comment 3

[Lars T Hansen]

(In reply to comment #2)
> Created attachment 530254 [details] [diff] [review] [review]
> Convert to VMPI_alloca

When landing this it will also be good to note, in the definition of ArraySort, that ArraySort is only supposed to be stack-allocated (it's the only way using VMPI_alloca for the field names array makes sense, lifetime-wise).

Comment 4

[Lars T Hansen]

Comment on attachment 530254 [details] [diff] [review]
Convert to VMPI_alloca

Redirecting "R?".

Comment 5

[Steven Johnson]

Comment on attachment 530254 [details] [diff] [review]
Convert to VMPI_alloca

Review of attachment 530254 [details] [diff] [review]:
-----------------------------------------------------------------

Scope creep: we should totally add a "VMPI_calloca" call to avoid the need for an explicit overflow check. (And then maybe deprecate VMPI_alloca.)

Comment 6

[Tamarin Bot]

changeset: 6295:d48515716429
user:      Lars T Hansen <[e-mail]>
summary:   Fix 654932 - Field names array in ArrayClass::generic_sortOn defeats reference counting (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/d48515716429

Comment 7

[Lars T Hansen]

lhansen-MacPro:tamarin-redux-serrano lhansen$ hg tip
changeset:   6241:f224b9f7e225
tag:         tip
user:        Lars T Hansen <[e-mail]>
date:        Tue May 10 09:45:02 2011 +0200
summary:     Fix 654932 - Field names array in ArrayClass::generic_sortOn defeats reference counting (r=stejohns)