r275600 - [objcmt] Fix a buffer overflow crash than can occur while modernizing enums.

Argyrios Kyrtzidis via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 15 13:48:17 PDT 2016


> On Jul 15, 2016, at 1:03 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> 
>> 
>> On Jul 15, 2016, at 12:58 PM, Ben Langmuir <blangmuir at apple.com <mailto:blangmuir at apple.com>> wrote:
>> 
>>> 
>>> On Jul 15, 2016, at 12:22 PM, Argyrios Kyrtzidis via cfe-commits <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>> wrote:
>>> 
>>> Author: akirtzidis
>>> Date: Fri Jul 15 14:22:34 2016
>>> New Revision: 275600
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=275600&view=rev <http://llvm.org/viewvc/llvm-project?rev=275600&view=rev>
>>> Log:
>>> [objcmt] Fix a buffer overflow crash than can occur while modernizing enums.
>>> 
>>> Note that due to the nature of the crash it requires libgmalloc or asan for it to crash consistently.
>>> 
>>> rdar://19932927 <rdar://19932927>
>>> 
>>> Added:
>>>   cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m
>>>   cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m.result
>>> Modified:
>>>   cfe/trunk/lib/ARCMigrate/ObjCMT.cpp
>>>   cfe/trunk/test/ARCMT/objcmt-ns-macros.m.result
>>> 
>>> Modified: cfe/trunk/lib/ARCMigrate/ObjCMT.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/ObjCMT.cpp?rev=275600&r1=275599&r2=275600&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/ARCMigrate/ObjCMT.cpp (original)
>>> +++ cfe/trunk/lib/ARCMigrate/ObjCMT.cpp Fri Jul 15 14:22:34 2016
>>> @@ -771,23 +771,11 @@ static void rewriteToNSMacroDecl(ASTCont
>>>  ClassString += ", ";
>>> 
>>>  ClassString += TypedefDcl->getIdentifier()->getName();
>>> -  ClassString += ')';
>>> -  SourceLocation EndLoc;
>>> -  if (EnumDcl->getIntegerTypeSourceInfo()) {
>>> -    TypeSourceInfo *TSourceInfo = EnumDcl->getIntegerTypeSourceInfo();
>>> -    TypeLoc TLoc = TSourceInfo->getTypeLoc();
>>> -    EndLoc = TLoc.getLocEnd();
>>> -    const char *lbrace = Ctx.getSourceManager().getCharacterData(EndLoc);
>>> -    unsigned count = 0;
>>> -    if (lbrace)
>>> -      while (lbrace[count] != '{')
>>> -        ++count;
>>> -    if (count > 0)
>>> -      EndLoc = EndLoc.getLocWithOffset(count-1);
>>> -  }
>>> -  else
>>> -    EndLoc = EnumDcl->getLocStart();
>>> -  SourceRange R(EnumDcl->getLocStart(), EndLoc);
>>> +  ClassString += ") ";
>>> +  SourceLocation EndLoc = EnumDcl->getBraceRange().getBegin();
>>> +  if (EndLoc.isInvalid())
>>> +    return;
>>> +  CharSourceRange R = CharSourceRange::getCharRange(EnumDcl->getLocStart(), EndLoc);
>>>  commit.replace(R, ClassString);
>>>  // This is to remove spaces between '}' and typedef name.
>>>  SourceLocation StartTypedefLoc = EnumDcl->getLocEnd();
>>> @@ -1900,18 +1888,20 @@ void ObjCMigrateASTConsumer::HandleTrans
>>>        if (++N == DEnd)
>>>          continue;
>>>        if (const EnumDecl *ED = dyn_cast<EnumDecl>(*N)) {
>>> -          if (++N != DEnd)
>>> -            if (const TypedefDecl *TDF = dyn_cast<TypedefDecl>(*N)) {
>>> -              // prefer typedef-follows-enum to enum-follows-typedef pattern.
>>> -              if (migrateNSEnumDecl(Ctx, ED, TDF)) {
>>> -                ++D; ++D;
>>> -                CacheObjCNSIntegerTypedefed(TD);
>>> -                continue;
>>> +          if (canModify(ED)) {
>> 
>> Is this change tested anywhere?  It looks orthogonal to the source range changes.
> 
> You are right that it is orthogonal. It avoids unnecessary work, it’s not changing behavior.

On second look, test case in r275609 shows behavior improvement.
Without this change, 'ARCMT/whitelisted/objcmt-with-whitelist.m’ will fail.

> 
>> 
>>> +            if (++N != DEnd)
>>> +              if (const TypedefDecl *TDF = dyn_cast<TypedefDecl>(*N)) {
>>> +                // prefer typedef-follows-enum to enum-follows-typedef pattern.
>>> +                if (migrateNSEnumDecl(Ctx, ED, TDF)) {
>>> +                  ++D; ++D;
>>> +                  CacheObjCNSIntegerTypedefed(TD);
>>> +                  continue;
>>> +                }
>>>              }
>>> +            if (migrateNSEnumDecl(Ctx, ED, TD)) {
>>> +              ++D;
>>> +              continue;
>>>            }
>>> -          if (migrateNSEnumDecl(Ctx, ED, TD)) {
>>> -            ++D;
>>> -            continue;
>>>          }
>>>        }
>>>        CacheObjCNSIntegerTypedefed(TD);
>>> 
>>> Added: cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m?rev=275600&view=auto <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m?rev=275600&view=auto>
>>> ==============================================================================
>>> --- cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m (added)
>>> +++ cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m Fri Jul 15 14:22:34 2016
>>> @@ -0,0 +1,14 @@
>>> +// RUN: rm -rf %t
>>> +// RUN: %clang_cc1 -objcmt-migrate-ns-macros -mt-migrate-directory %t %s -x objective-c -fobjc-runtime-has-weak -fobjc-arc -triple x86_64-apple-darwin11
>>> +// RUN: c-arcmt-test -mt-migrate-directory %t | arcmt-test -verify-transformed-files %s.result
>>> +
>>> +#define NS_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
>>> +#define NS_OPTIONS(_type, _name) enum _name : _type _name; enum _name : _type
>>> +typedef long NSInteger;
>>> +
>>> +typedef enum : NSInteger {five} ApplicableEnum;
>>> +
>>> +typedef unsigned long mytd;
>>> +
>>> +#define MY_ENUM(name, type, ...) typedef enum : type { __VA_ARGS__ } name##_t
>>> +MY_ENUM(MyEnum, unsigned int, One);
>>> 
>>> Added: cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m.result
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m.result?rev=275600&view=auto <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m.result?rev=275600&view=auto>
>>> ==============================================================================
>>> --- cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m.result (added)
>>> +++ cfe/trunk/test/ARCMT/objcmt-ns-enum-crash.m.result Fri Jul 15 14:22:34 2016
>>> @@ -0,0 +1,14 @@
>>> +// RUN: rm -rf %t
>>> +// RUN: %clang_cc1 -objcmt-migrate-ns-macros -mt-migrate-directory %t %s -x objective-c -fobjc-runtime-has-weak -fobjc-arc -triple x86_64-apple-darwin11
>>> +// RUN: c-arcmt-test -mt-migrate-directory %t | arcmt-test -verify-transformed-files %s.result
>>> +
>>> +#define NS_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
>>> +#define NS_OPTIONS(_type, _name) enum _name : _type _name; enum _name : _type
>>> +typedef long NSInteger;
>>> +
>>> +typedef NS_ENUM(NSInteger, ApplicableEnum) {five};
>>> +
>>> +typedef unsigned long mytd;
>>> +
>>> +#define MY_ENUM(name, type, ...) typedef enum : type { __VA_ARGS__ } name##_t
>>> +MY_ENUM(MyEnum, unsigned int, One);
>>> 
>>> Modified: cfe/trunk/test/ARCMT/objcmt-ns-macros.m.result
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/objcmt-ns-macros.m.result?rev=275600&r1=275599&r2=275600&view=diff <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/objcmt-ns-macros.m.result?rev=275600&r1=275599&r2=275600&view=diff>
>>> ==============================================================================
>>> --- cfe/trunk/test/ARCMT/objcmt-ns-macros.m.result (original)
>>> +++ cfe/trunk/test/ARCMT/objcmt-ns-macros.m.result Fri Jul 15 14:22:34 2016
>>> @@ -77,7 +77,7 @@ typedef NS_ENUM(NSInteger, UIK) {
>>>  UIKTwo = 2,
>>> };
>>> 
>>> -typedef NS_ENUM(unsigned int, NSTickMarkPosition)  {
>>> +typedef NS_ENUM(unsigned int, NSTickMarkPosition) {
>>>    NSTickMarkBelow = 0,
>>>    NSTickMarkAbove = 1,
>>>    NSTickMarkLeft = NSTickMarkAbove,
>>> 
>>> 
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160715/89094f7a/attachment-0001.html>


More information about the cfe-commits mailing list