[PATCH] Fix crash in CheckObjCCircularContainer

[AlexDenisov]

> 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: proper_fix_for_circular_containers_v2.patch
Type: application/octet-stream
Size: 15311 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150805/ac2a6946/attachment-0001.obj>
-------------- next part --------------

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150805/ac2a6946/attachment-0001.sig>