[PATCH] Fix crash in CheckObjCCircularContainer

[Argyrios Kyrtzidis]

> +    IdentifierInfo *II = &S.Context.Idents.get(InterfaceDecl->getName());


You can just do “InterfaceDecl->getIdentifier()”, no need to lookup by string. Also this will eliminate the need to pass Sema as parameter.
And ‘isSubclassOfNSClass()’ seems generally useful, how about you make it a function of NSAPI ?

Otherwise LGTM.


> On Aug 5, 2015, at 1:11 PM, AlexDenisov <1101.debian at gmail.com> wrote:
> 
>> Why not get the IdentifierInfo pointer for the class name from the NSAPI object and compare that ?
> There are more than one way to do things, it’s just lack of knowledge about the code base.
> 
>> Also there is code duplication, since the same code pattern is used in 3 places, could you refactor into a function ?
> 
> Fixed this as well. Also, I got rid of `NSCountedSet` since it’s a subclass of `NSMutableSet` and will be caught by `isSubclassOfNSClass`.
> 
> The new version of the patch attached.
> --
> AlexDenisov
> Software Engineer, http://lowlevelbits.org
> 
> <proper_fix_for_circular_containers_v2.patch>
>> On 05 Aug 2015, at 19:22, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>> 
>>> 
>>> -  if (S.NSMutableArrayPointer != Message->getReceiverType()) {
>>> +  ObjCInterfaceDecl *Receiver = Message->getReceiverInterface();
>>> +  if (!Receiver) {
>>> +    return None;
>>> +  }
>>> +
>>> +  bool IsMutableArray = false;
>>> +  do {
>>> +    QualType QT = S.Context.getObjCInterfaceType(Receiver);
>>> +    QualType ReceiverType = S.Context.getObjCObjectPointerType(QT);
>>> +
>>> +    IsMutableArray = !S.NSMutableArrayPointer.isNull() &&
>>> +      ReceiverType == S.NSMutableArrayPointer;
>>> +
>>> +    if (IsMutableArray) {
>>> +      break;
>>> +    }
>>> +  } while ((Receiver = Receiver->getSuperClass()));
>>> +
>>> +  if (!IsMutableArray) {
>>>    return None;
>>>  }
>>> 
>> 
>> Why not get the IdentifierInfo pointer for the class name from the NSAPI object and compare that ? It seems unnecessary to be doing the type lookups for this.
>> 
>> Also there is code duplication, since the same code pattern is used in 3 places, could you refactor into a function ?
>> 
>> 
>> 
>>> On Jul 31, 2015, at 1:55 PM, AlexDenisov <1101.debian at gmail.com> wrote:
>>> 
>>>> To clarify, are you saying that the warning may lead to false positives when used in subclasses ?
>>> 
>>> Seems I was wrong.
>>> Just checked the behaviour with backing storage - it also leads to a circular container problem.
>>> 
>>> Also, you can find attachment with a ‘proper’ implementation, which also covers subclassing.
>>> 
>>> P.S. I didn’t measure performance, but I think this implementation might have negative impact on the speed.
>>> --
>>> AlexDenisov
>>> Software Engineer, http://lowlevelbits.org
>>> 
>>> <proper_fix_for_circular_containers.patch>
>>>> On 30 Jul 2015, at 18:18, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>>> 
>>>> 
>>>>> On Jul 30, 2015, at 1:05 AM, AlexDenisov <1101.debian at gmail.com> wrote:
>>>>> 
>>>>> The patch is a simplest fix for crash when CheckObjCCircularContainer
>>>>> applies to a message to a ’super’, e.g.:
>>>>> 
>>>>> @implementation Foo : NSMutableArray
>>>>> - foo {
>>>>> [super addObject:nil];
>>>>> }
>>>>> @end
>>>>> 
>>>>> 
>>>>> This is, probably, not a proper fix for the problem,
>>>>> but initial patch wasn’t intended to apply checks to any kind
>>>>> of subclassing, because it, imho, over-complicates implementation:
>>>> 
>>>> To clarify, are you saying that the warning may lead to false positives when used in subclasses ?
>>>> If that’s the case could we just disable it inside collection subclasses, at least until the false positives can be addressed ?
>>>> 
>>>>> 
>>>>> This particular problem touches subclassing from a class-cluster,
>>>>> which means that the concrete subclass will have some backing storage, e.g.:
>>>>> 
>>>>> @implementation FootableArray : NSMutableArray
>>>>> {
>>>>> NSMutableArray *_backingStorage;
>>>>> }
>>>>> 
>>>>> - addObject:(id)object {
>>>>> [_backingStorage addObject:object];
>>>>> }
>>>>> 
>>>>> @end
>>>>> 
>>>>> In this case even adding `self` to `self` would not lead to a circular container:
>>>>> 
>>>>> - foo {
>>>>> [self addObject:self]; // puts `self` into the `_backingStorage`
>>>>> }
>>>>> 
>>>>> I would apply this patch as is and postpone a ‘proper and bullet-proof implementation’
>>>>> when I, or somebody who is also interested, will have more time.
>>>>> 
>>>>> If there are any questions/suggestions/objections - let’s discuss them.
>>>>> --
>>>>> AlexDenisov
>>>>> Software Engineer, http://lowlevelbits.org
>>>>> 
>>>>> 
>>>>> <fix_circular_container_crash.patch>
>>>> 
>>> 
>> 
>