Re: Crash in countByEnumeratingWithState method of GNUstep's implementation of NSArray

[Richard Frith-Macdonald]

On 8 Jan 2014, at 12:34, Mathias Bauer <mathias_bauer@gmx.net> wrote:

> I doubt that returning the address of a stack variable will fix that. "size" 
> must become an iVar, IMHO.
> 
> Regards,
> Mathias
> 
> Am 08.01.14 13:21, schrieb Quentin Mathé:
>> Hi Matthias,
>> 
>> Le 8 janv. 2014 à 10:45, Mathias Bauer a écrit :
>> 
>>> Hi,
>>> 
>>> it seems that the implementation of countByEnumeratingWithState in NSArray 
>>> is broken.
>>> 
>>> The following code in NSArray.m
>>> 
>>>> {
>>>>   NSUInteger size = [self count];
>>>>   NSInteger count;
>>>> 
>>>>   /* This is cached in the caller at the start and compared at each
>>>>    * iteration.   If it changes during the iteration then
>>>>    * objc_enumerationMutation() will be called, throwing an exception.
>>>>    */
>>>>   state->mutationsPtr = (unsigned long *)size;
>>> 
>>> of course crashes as soon as any fast enumeration is executed for any 
>>> collection deriving from NSArray. The cast in the last line can't work.
>>> 
>>> Now I'm wondering how this problem could remain undiscovered or at least 
>>> unfixed for such a long time. I doubt that everybody who implemented a 
>>> class that derives from NSArray also re-implemented this method.
>> 
>> I just stumbled on it today while testing some custom NSArray subclass. I 
>> think most people don't write NSArray subclass, and GNUstep concrete 
>> subclasses are all overriding the fast enumeration method, so the default 
>> fast enumeration implementation in NSArray was just never executed.
>> 
>>> A simple fix would be to add an iVar that gets the result of [self count] 
>>> each time this method is called and assigning its address to 
>>> state->mutationsPtr.
>> 
>> The following should be enough to fix it: state->mutationsPtr = (unsigned 
>> long *)&size;
>> 
>>> Any chance for getting this fixed in the trunk version?
>> 
>> I'll commit this fix today.

I don't use fast enumeration, so I'm unfamiliar with this, but on the basis of 
looking at this code and reading a little it also seems strange to me to use a 
stack variable, unless you can somehow guarantee the stack will be left intact 
while the fast enumeration takes place?

I wonder if the pointer should simply be set to self?  That would presumably 
point to something guaranteed to exist for the duration fo the enumeration and 
presumably to remain unchanged.  This ought to be fine for NSArray which is 
never mutated.

I don't understand the point of using an ivar though.  Leaving aside the fact 
that NSArray is an abstract class and is not supposed/allowed to have ivars, 
what would be the point? As I understand it the pointer is supposed to be set 
to point to some value which will change if the collection is mutated ... that 
should never happen for NSArray and if someone implements an NSMutableArray 
subclass they would presumably want to implement their own storage and 
mechanism to indicate when their contents had changed.  So it looks to me like 
subclasses of NSMutableArray ought to be implementing their own version of this 
method.

Of course, you may know better ... have Apple added an API to NSMutableArray 
which allows a subclass to mark instances of itsself as mutated?