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

[Quentin Mathé]

Le 8 janv. 2014 à 14:02, Richard Frith-Macdonald a écrit :

> On 8 Jan 2014, at 12:34, Mathias Bauer <mathias_bauer@gmx.net> wrote:
> 
>> Am 08.01.14 13:21, schrieb Quentin Mathé:
>>> 
>>> Le 8 janv. 2014 à 10:45, Mathias Bauer a écrit :
>>> 
>>>> 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;
> 
> 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?

Yes, using a stack variable doesn't make sense. state->mutationsPtr would 
probably point to some random content in memory once 
-countWithEnumeratingWithState: returns.

> 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.

That sounds reasonable.

> 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?

It seems there is no such API.

I was suggesting in my previous reply to move the _version ivar from 
GSMutableArray to NSMutableArray, but as you pointed it out, this won't help 
since overriden primitive methods (e.g. -addObject:) need to increment this 
variable. And Apple doesn't declare any similar protected ivar or special 
method for this purpose.

I tested writing a custom NSMutableArray subclass and mutating it during fast 
enumeration on Mac OS X, and I get no mutation-related exceptions, so I guess 
NSMutableArray inherit -countByEnumeratingWithState: from NSArray in Cocoa (if 
you don't override it).

Cheers,
Quentin.