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

[Quentin Mathé]

Le 8 janv. 2014 à 13:34, Mathias Bauer a écrit :

> I doubt that returning the address of a stack variable will fix that. "size" 
> must become an iVar, IMHO.

Right, I typed this a bit quickly. 

GSMutableArray declares a _version ivar for this purpose, so it could probably 
be moved up to NSMutableArray. We could fix -countByEnumeratingWithState: in 
NSArray to set the mutationsPtr to 'self', and override  
-countByEnumeratingWithState: in NSMutableArray to use _version.

The problem is that all NSMutableArray mutation methods have to be updated to 
increment _version. So I'll wait to have some more feedback before committing 
any changes.

Quentin.

> 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.
>> 
>> Cheers,
>> Quentin.
>>