From a8897c241251a8e1f1edaea96baef606ededd996 Mon Sep 17 00:00:00 2001 From: Jia Chen Date: Fri, 21 Jul 2017 11:24:01 -0700 Subject: [PATCH] Properly model the copy semantics of NSString.stringWithUTF8String and NSString.stringWithString. Summary: Both `stringWithUTF8String` and `stringWithString` implements copy semantics that copies the content of their parameter into a newly allocated buffer. We modeled this as pointer assignment in the past, which means that once we write ``` NSString* foo() { char buf[...]; ... return [NSString stringWithUTF8String:buf]; } ``` We are going to get a spurious stack variable address escape report because local pointer `buf` is assigned to the newly created string and the string gets returned. This diff tries to address the issue by heap-allocating a buffer and `memcpy` the contents in `stringWithUTF8String` and `stringWithString`. But this change will create another problem: the allocated buffer will be reported as leaked by the backend, while in reality those buffers won't actually be leaked as they are allocated in a region that will be periodically autoreleased. To suppress spurious memory leak FPs, I added another attribute `Awont_leak` that will suppress the leakage report on any expressions that get tagged with it. Reviewed By: jeremydubreil Differential Revision: D5403084 fbshipit-source-id: df6de7f --- infer/models/objc/src/NSString.h | 1 + infer/models/objc/src/NSString.m | 24 ++++++++++++++-- infer/src/IR/BUILTINS.mli | 2 ++ infer/src/IR/BuiltinDecl.ml | 2 ++ infer/src/IR/PredSymb.ml | 8 ++++++ infer/src/IR/PredSymb.mli | 4 +++ infer/src/backend/Attribute.ml | 2 ++ infer/src/backend/Attribute.mli | 3 ++ infer/src/backend/BuiltinDefn.ml | 7 +++++ infer/src/backend/abs.ml | 28 ++++++++++++------- .../codetoanalyze/objc/errors/issues.exp | 1 - .../NSString_models_tests.m | 11 ++++++++ 12 files changed, 79 insertions(+), 14 deletions(-) diff --git a/infer/models/objc/src/NSString.h b/infer/models/objc/src/NSString.h index f13603027..cdc40b3f2 100644 --- a/infer/models/objc/src/NSString.h +++ b/infer/models/objc/src/NSString.h @@ -19,4 +19,5 @@ + (instancetype)localizedStringWithFormat:(NSString*)format, ...; - (instancetype)initWithFormat:(NSString*)format, ...; - (instancetype)initWithFormat:(NSString*)format arguments:(va_list)argList; +- (void)dealloc; @end diff --git a/infer/models/objc/src/NSString.m b/infer/models/objc/src/NSString.m index 84850c327..4cea368db 100644 --- a/infer/models/objc/src/NSString.m +++ b/infer/models/objc/src/NSString.m @@ -10,22 +10,33 @@ #import "NSString.h" #import -void __get_array_length(const UInt8); +size_t __get_array_length(const void* arr); void __infer_assume(bool cond); +void __set_wont_leak_attribute(const void* ptr); @implementation NSString + (instancetype)stringWithUTF8String:(const char*)bytes { // using alloc as the documentation doesn't say it may return nil NSString* s = [NSString alloc]; - s->value = bytes; + int len; + len = __get_array_length(bytes); + s->value = (const char*)malloc(len); + // The newly allocated string will be autoreleased by the runtime + __set_wont_leak_attribute(s->value); + memcpy(s->value, bytes, len); return s; } + (instancetype)stringWithString:(NSString*)aString { NSString* s = [NSString alloc]; - s->value = aString->value; + int len; + len = __get_array_length(aString->value); + s->value = (const char*)malloc(len); + // The newly allocated string will be autoreleased by the runtime + __set_wont_leak_attribute(s->value); + memcpy(s->value, aString->value, len); return s; } @@ -70,4 +81,11 @@ void __infer_assume(bool cond); } } +- (void)dealloc { + if (self != nil && self->value != 0) { + free(self->value); + } + [super dealloc]; +} + @end diff --git a/infer/src/IR/BUILTINS.mli b/infer/src/IR/BUILTINS.mli index 3e2c6a29e..fa7e1b4b3 100644 --- a/infer/src/IR/BUILTINS.mli +++ b/infer/src/IR/BUILTINS.mli @@ -107,6 +107,8 @@ module type S = sig val __set_untaint_attribute : t + val __set_wont_leak_attribute : t + val __split_get_nth : t val __throw : t diff --git a/infer/src/IR/BuiltinDecl.ml b/infer/src/IR/BuiltinDecl.ml index 6502f4d85..98b85c673 100644 --- a/infer/src/IR/BuiltinDecl.ml +++ b/infer/src/IR/BuiltinDecl.ml @@ -123,6 +123,8 @@ let __set_unsubscribed_observer_attribute = create_procname "__set_unsubscribed_ let __set_untaint_attribute = create_procname "__set_untaint_attribute" +let __set_wont_leak_attribute = create_procname "__set_wont_leak_attribute" + let __split_get_nth = create_procname "__split_get_nth" let __throw = create_procname "__throw" diff --git a/infer/src/IR/PredSymb.ml b/infer/src/IR/PredSymb.ml index ac64c16ed..efcd84b70 100644 --- a/infer/src/IR/PredSymb.ml +++ b/infer/src/IR/PredSymb.ml @@ -121,6 +121,7 @@ type t = | Aobserver (** denotes an object registered as an observers to a notification center *) | Aunsubscribed_observer (** denotes an object unsubscribed from observers of a notification center *) + | Awont_leak (** value do not participate in memory leak analysis *) [@@deriving compare] let equal = [%compare.equal : t] @@ -158,6 +159,7 @@ type category = | ACundef | ACretval | ACobserver + | ACwontleak [@@deriving compare] let equal_category = [%compare.equal : category] @@ -182,9 +184,13 @@ let to_category att = -> ACundef | Aobserver | Aunsubscribed_observer -> ACobserver + | Awont_leak + -> ACwontleak let is_undef = function Aundef _ -> true | _ -> false +let is_wont_leak = function Awont_leak -> true | _ -> false + (** convert the attribute to a string *) let to_string pe = function | Aresource ra @@ -254,6 +260,8 @@ let to_string pe = function -> "OBSERVER" | Aunsubscribed_observer -> "UNSUBSCRIBED_OBSERVER" + | Awont_leak + -> "WONT_LEAK" (** dump an attribute *) let d_attribute (a: t) = L.add_print_action (L.PTattribute, Obj.repr a) diff --git a/infer/src/IR/PredSymb.mli b/infer/src/IR/PredSymb.mli index 8a39cbcff..8f0128b42 100644 --- a/infer/src/IR/PredSymb.mli +++ b/infer/src/IR/PredSymb.mli @@ -96,6 +96,7 @@ type t = | Aobserver (** denotes an object registered as an observers to a notification center *) | Aunsubscribed_observer (** denotes an object unsubscribed from observers of a notification center *) + | Awont_leak (** value do not participate in memory leak analysis *) [@@deriving compare] val equal : t -> t -> bool @@ -117,6 +118,7 @@ type category = | ACundef | ACretval | ACobserver + | ACwontleak [@@deriving compare] val equal_category : category -> category -> bool @@ -126,6 +128,8 @@ val to_category : t -> category val is_undef : t -> bool +val is_wont_leak : t -> bool + val to_string : Pp.env -> t -> string (** convert the attribute to a string *) diff --git a/infer/src/backend/Attribute.ml b/infer/src/backend/Attribute.ml index eba05aee9..12fc44479 100644 --- a/infer/src/backend/Attribute.ml +++ b/infer/src/backend/Attribute.ml @@ -107,6 +107,8 @@ let get_observer tenv prop exp = get tenv prop exp ACobserver let get_retval tenv prop exp = get tenv prop exp ACretval +let get_wontleak tenv prop exp = get tenv prop exp ACwontleak + let has_dangling_uninit tenv prop exp = let la = get_for_exp tenv prop exp in List.exists diff --git a/infer/src/backend/Attribute.mli b/infer/src/backend/Attribute.mli index 12384b95b..5bb9f8abf 100644 --- a/infer/src/backend/Attribute.mli +++ b/infer/src/backend/Attribute.mli @@ -65,6 +65,9 @@ val get_taint : Tenv.t -> 'a Prop.t -> Exp.t -> Sil.atom option val get_undef : Tenv.t -> 'a Prop.t -> Exp.t -> Sil.atom option (** Get the undef attribute associated to the expression, if any *) +val get_wontleak : Tenv.t -> 'a Prop.t -> Exp.t -> Sil.atom option +(** Get the wontleak attribute associated to the expression, if any *) + val has_dangling_uninit : Tenv.t -> 'a Prop.t -> Exp.t -> bool (** Test for existence of an Adangling DAuninit attribute associated to the exp *) diff --git a/infer/src/backend/BuiltinDefn.ml b/infer/src/backend/BuiltinDefn.ml index ce1378867..73075589f 100644 --- a/infer/src/backend/BuiltinDefn.ml +++ b/infer/src/backend/BuiltinDefn.ml @@ -636,6 +636,10 @@ let execute___set_unlocked_attribute ({Builtin.pdesc; loc} as builtin_args) : Bu in execute___set_attr (PredSymb.Aresource ra) builtin_args +(** Set the attibute of the value as wont leak*) +let execute___set_wont_leak_attribute builtin_args : Builtin.ret_typ = + execute___set_attr PredSymb.Awont_leak builtin_args + (** Set the attibute of the value as tainted *) let execute___set_taint_attribute {Builtin.tenv; pdesc; args; prop_; path} : Builtin.ret_typ = match args with @@ -1189,6 +1193,9 @@ let __set_unsubscribed_observer_attribute = let __set_untaint_attribute = Builtin.register BuiltinDecl.__set_untaint_attribute execute___set_untaint_attribute +let __set_wont_leak_attribute = + Builtin.register BuiltinDecl.__set_wont_leak_attribute execute___set_wont_leak_attribute + (* splits a string given a separator and returns the nth string *) let __split_get_nth = Builtin.register BuiltinDecl.__split_get_nth execute___split_get_nth diff --git a/infer/src/backend/abs.ml b/infer/src/backend/abs.ml index a4ba059d2..3bd0e7574 100644 --- a/infer/src/backend/abs.ml +++ b/infer/src/backend/abs.ml @@ -1232,18 +1232,24 @@ let check_junk ?original_prop pname tenv prop = let res = ref None in let do_entry e = check_observer_is_unsubscribed_deallocation tenv prop e ; - match Attribute.get_resource tenv prop e with - | Some Apred ((Aresource {ra_kind= Racquire} as a), _) - -> L.d_str "ATTRIBUTE: " ; - PredSymb.d_attribute a ; - L.d_ln () ; + match Attribute.get_wontleak tenv prop e with + | Some Apred ((Awont_leak as a), _) + -> L.d_strln "WONT_LEAK" ; res := Some a | _ -> - match Attribute.get_undef tenv prop e with - | Some Apred ((Aundef _ as a), _) - -> res := Some a - | _ - -> () + match Attribute.get_resource tenv prop e with + | Some Apred ((Aresource {ra_kind= Racquire} as a), _) + -> L.d_str "ATTRIBUTE: " ; + PredSymb.d_attribute a ; + L.d_ln () ; + res := Some a + | _ -> + match Attribute.get_undef tenv prop e with + | Some Apred ((Aundef _ as a), _) + -> L.d_strln "UNDEF" ; + res := Some a + | _ + -> () in List.iter ~f:do_entry entries ; !res in @@ -1285,6 +1291,8 @@ let check_junk ?original_prop pname tenv prop = in let ignore_resource, exn = match (alloc_attribute, resource) with + | Some PredSymb.Awont_leak, Rmemory _ + -> (true, exn_leak) | Some _, Rmemory Mobjc when hpred_in_cycle hpred -> (* When there is a cycle in objc we ignore it only if it's empty or it has weak or unsafe_unretained fields. diff --git a/infer/tests/codetoanalyze/objc/errors/issues.exp b/infer/tests/codetoanalyze/objc/errors/issues.exp index 7f044201e..1cf9441e4 100644 --- a/infer/tests/codetoanalyze/objc/errors/issues.exp +++ b/infer/tests/codetoanalyze/objc/errors/issues.exp @@ -91,7 +91,6 @@ codetoanalyze/objc/shared/memory_leaks_benchmark/TollBridgeExample.m, TollBridge codetoanalyze/objc/shared/memory_leaks_benchmark/TollBridgeExample.m, TollBridgeExample_bridge, 2, MEMORY_LEAK, [start of procedure bridge] codetoanalyze/objc/shared/memory_leaks_benchmark/TollBridgeExample.m, bridgeDictionaryNoLeak, 1, UNINITIALIZED_VALUE, [start of procedure bridgeDictionaryNoLeak()] codetoanalyze/objc/errors/memory_leaks_benchmark/NSData_models_tests.m, NSData_models_tests_macForIV:, 2, MEMORY_LEAK, [start of procedure macForIV:] -codetoanalyze/objc/errors/memory_leaks_benchmark/NSString_models_tests.m, createURLQueryStringBodyEscaping, 6, PRECONDITION_NOT_MET, [start of procedure createURLQueryStringBodyEscaping(),Condition is true] codetoanalyze/objc/errors/memory_leaks_benchmark/NSString_models_tests.m, createURLQueryStringBodyEscaping, 11, UNINITIALIZED_VALUE, [start of procedure createURLQueryStringBodyEscaping(),Condition is false] codetoanalyze/objc/errors/memory_leaks_benchmark/RetainReleaseExampleBucketing.m, RetainReleaseTest, 0, RETURN_VALUE_IGNORED, [start of procedure RetainReleaseTest()] codetoanalyze/objc/errors/npe/Fraction.m, test_virtual_call, 7, NULL_DEREFERENCE, [start of procedure test_virtual_call(),start of procedure setNumerator:,return from a call to Fraction_setNumerator:,start of procedure getNumerator,return from a call to Fraction_getNumerator,Condition is true] diff --git a/infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/NSString_models_tests.m b/infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/NSString_models_tests.m index 3daf462e5..316cc3e34 100644 --- a/infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/NSString_models_tests.m +++ b/infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/NSString_models_tests.m @@ -30,6 +30,17 @@ NSString* createURLQueryStringBodyEscaping(NSDictionary* parameters, return resultString; } +NSString* stringWithUTF8StringNoLeakOk(const char* buf) { + return [NSString stringWithUTF8String:buf]; +} +NSString* stringWithStringNoLeakOk(const char* buf) { + return [NSString stringWithString:buf]; +} + +- (void)NSAssertNoLeakOk { + NSAssert(true, @"This string does not create any memory leak"); +} + - (NSString*)hexStringValue { size_t hexLen = 2 * 10 * sizeof(char); char* outString = (char*)malloc(hexLen + 1);