[nstring-internal-ptr] Adding a new issue type NSSTRING_INTERNAL_PTR_…

…CAPTURED_IN_BLOCK

Summary:
The issue is that 	when the result of a call to the method `cStringUsingEncoding` of `NSString` or its property `UTF8String`
is captured in an escaping block without copying it first, this can cause a crash.

We adapt the checker for finding CXX String Captured In Block to deal with this case too as both cases are very similar.

First,  we add the context info to the captured variables in the preanalysis, and then flag it in the checker.

Reviewed By: geralt-encore

Differential Revision: D65070300

fbshipit-source-id: 87dde7d90e8e22f69ad23afe78cdea90c50f5c23

infer/documentation/issues/NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK.md

@@ -0,0 +1,23 @@
+This check flags when the result of a call to the method `cStringUsingEncoding` of `NSString` or its property `UTF8String`
+is captured in an escaping block. In both cases this C string is a pointer to a structure inside the string object, which
+may have a lifetime shorter than the string object and will certainly not have a longer lifetime. Therefore, you should copy
+the C string if it needs to be stored outside of the memory context in which you use it.
+
+Example:
+
+```
+  char* encoding = (char*)[name cStringUsingEncoding:NSUTF8StringEncoding];
+  dispatch_async(dispatch_get_main_queue(), ^{
+    const char* c = encoding; //bug
+  });
+
+
+
+  char* utf8string = name.UTF8String;
+  dispatch_async(dispatch_get_main_queue(), ^{
+    const char* c = utf8string; //bug
+  });
+
+```
+
+This could cause crashes because the variable is likely to be freed when the code is executed, leaving the pointer dangling.

infer/man/man1/infer-full.txt

@@ -693,6 +693,7 @@ OPTIONS
              NO_MATCH_OF_RHS_LATENT (disabled by default),
              NO_TRUE_BRANCH_IN_IF (enabled by default),
              NO_TRUE_BRANCH_IN_IF_LATENT (disabled by default),
+             NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK (disabled by default),
              NULLPTR_DEREFERENCE (enabled by default),
              NULLPTR_DEREFERENCE_IN_NULLSAFE_CLASS (enabled by default),
              NULLPTR_DEREFERENCE_IN_NULLSAFE_CLASS_LATENT (disabled by

infer/man/man1/infer-report.txt

@@ -211,6 +211,7 @@ OPTIONS
              NO_MATCH_OF_RHS_LATENT (disabled by default),
              NO_TRUE_BRANCH_IN_IF (enabled by default),
              NO_TRUE_BRANCH_IN_IF_LATENT (disabled by default),
+             NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK (disabled by default),
              NULLPTR_DEREFERENCE (enabled by default),
              NULLPTR_DEREFERENCE_IN_NULLSAFE_CLASS (enabled by default),
              NULLPTR_DEREFERENCE_IN_NULLSAFE_CLASS_LATENT (disabled by

infer/man/man1/infer.txt

@@ -693,6 +693,7 @@ OPTIONS
              NO_MATCH_OF_RHS_LATENT (disabled by default),
              NO_TRUE_BRANCH_IN_IF (enabled by default),
              NO_TRUE_BRANCH_IN_IF_LATENT (disabled by default),
+             NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK (disabled by default),
              NULLPTR_DEREFERENCE (enabled by default),
              NULLPTR_DEREFERENCE_IN_NULLSAFE_CLASS (enabled by default),
              NULLPTR_DEREFERENCE_IN_NULLSAFE_CLASS_LATENT (disabled by

infer/src/base/IssueType.ml

@@ -793,6 +793,12 @@ let no_matching_branch_in_try =
     ~user_documentation:[%blob "./documentation/issues/NO_MATCHING_BRANCH_IN_TRY.md"]
 
 
+let ns_string_captured_in_block =
+  register ~category:MemoryError ~enabled:false ~id:"NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK"
+    ~hum:"NSString Captured in Block" Error SelfInBlock
+    ~user_documentation:[%blob "./documentation/issues/CXX_STRING_CAPTURED_IN_BLOCK.md"]
+
+
 let null_argument =
   register_with_latent ~category:RuntimeException ~id:"NULL_ARGUMENT" Error Pulse
     ~user_documentation:[%blob "./documentation/issues/NULL_ARGUMENT.md"]

infer/src/base/IssueType.mli

@@ -278,6 +278,8 @@ val no_true_branch_in_if : latent:bool -> t
 
 val no_matching_branch_in_try : latent:bool -> t
 
+val ns_string_captured_in_block : t
+
 val null_argument : latent:bool -> t
 
 val null_dereference : t

infer/src/checkers/ComputeCapturedInfo.ml

@@ -50,36 +50,26 @@ module CheckedForNull = struct
       {astate with vars}
 
 
-    let call args astate =
-      let update_captured_in_closure closure_pname astate (exp, _) =
-        match exp with
-        | Exp.Var id -> (
-          match find_block_param id astate with
-          | Some name ->
-              let pvars =
-                PVars.update name
-                  (fun data_opt ->
-                    match data_opt with
-                    | Some data ->
-                        Some {data with captured_in_closure= Some closure_pname}
-                    | None ->
-                        Some {checked= false; captured_in_closure= Some closure_pname} )
-                  astate.pvars
-              in
-              {astate with pvars}
-          | None ->
-              astate )
-        | _ ->
-            astate
+    let call exp astate =
+      let update_captured_in_closure closure_pname astate (_, captured_var) =
+        let pvars =
+          PVars.update
+            (Pvar.get_name captured_var.CapturedVar.pvar)
+            (fun data_opt ->
+              match data_opt with
+              | Some data ->
+                  Some {data with captured_in_closure= Some closure_pname}
+              | None ->
+                  Some {checked= false; captured_in_closure= Some closure_pname} )
+            astate.pvars
+        in
+        {astate with pvars}
       in
-      let process_arg astate (exp, _) =
-        match exp with
-        | Exp.Closure {name; captured_vars} ->
-            List.fold ~f:(update_captured_in_closure name) captured_vars ~init:astate
-        | _ ->
-            astate
-      in
-      List.fold ~f:process_arg args ~init:astate
+      match exp with
+      | Exp.Closure {name; captured_vars} ->
+          List.fold ~f:(update_captured_in_closure name) captured_vars ~init:astate
+      | _ ->
+          astate
   end
 
   module Domain = struct
@@ -118,7 +108,11 @@ module CheckedForNull = struct
         | _ ->
             astate )
       | Call (_, Exp.Const (Const.Cfun _procname), args, _loc, _call_flags) ->
-          Domain.call args astate
+          List.fold_right ~f:Domain.call (List.map ~f:(fun el -> fst el) args) ~init:astate
+      | Call (_, (Exp.Closure _ as exp), _args, _loc, _call_flags) ->
+          Domain.call exp astate
+      | Store {e2} ->
+          Domain.call e2 astate
       | _ ->
           astate
   end
@@ -151,22 +145,9 @@ module InternalStringPointer = struct
 
     let pp fmt {vars; pvars} = F.fprintf fmt "Vars= %a@\nPVars= %a@\n" Vars.pp vars PVars.pp pvars
 
-    let is_local pvar proc_desc =
-      let attributes = Procdesc.get_attributes proc_desc in
-      let formals = attributes.ProcAttributes.formals in
-      let is_formal name =
-        List.exists
-          ~f:(fun (formal, typ, _) -> Mangled.equal formal name && Typ.is_pointer_to_function typ)
-          formals
-      in
-      (not (Pvar.is_global pvar)) && not (is_formal (Pvar.get_name pvar))
-
-
-    let set_internal_pointer var pvar typ proc_desc astate =
-      if is_local pvar proc_desc then
-        let vars = Vars.add var {VarsDomainData.internal_pointer_of= typ} astate.vars in
-        {astate with vars}
-      else astate
+    let set_internal_pointer var typ astate =
+      let vars = Vars.add var {VarsDomainData.internal_pointer_of= typ} astate.vars in
+      {astate with vars}
 
 
     let is_internal_pointer_of pvar astate =
@@ -189,9 +170,7 @@ module InternalStringPointer = struct
   module Domain = struct
     include AbstractDomain.FiniteSet (Mem)
 
-    let set_internal_pointer var pvar typ proc_desc astate =
-      map (Mem.set_internal_pointer var pvar typ proc_desc) astate
-
+    let set_internal_pointer var typ astate : t = map (Mem.set_internal_pointer var typ) astate
 
     let is_internal_pointer_of pvar astate =
       fold
@@ -217,18 +196,45 @@ module InternalStringPointer = struct
       QualifiedCppName.Match.of_fuzzy_qual_names ["std::basic_string::c_str"]
 
 
+    let cstring_using_encoding =
+      QualifiedCppName.Match.of_fuzzy_qual_names ["NSString::cStringUsingEncoding:"]
+
+
+    let utf8string = QualifiedCppName.Match.of_fuzzy_qual_names ["NSString::UTF8String"]
+
+    let is_local pvar proc_desc =
+      let attributes = Procdesc.get_attributes proc_desc in
+      let formals = attributes.ProcAttributes.formals in
+      let is_formal name =
+        List.exists
+          ~f:(fun (formal, typ, _) -> Mangled.equal formal name && Typ.is_pointer_to_function typ)
+          formals
+      in
+      (not (Pvar.is_global pvar)) && not (is_formal (Pvar.get_name pvar))
+
+
     let exec_instr (astate : Domain.t) proc_desc _cfg_node _ (instr : Sil.instr) =
       match instr with
       | Store {e1= Lvar pvar; e2= Exp.Var id} ->
           Domain.store pvar id astate
-      | Call ((var, _), Exp.Const (Const.Cfun procname), [(Lvar pvar, typ)], _, _) ->
+      | Call ((var, _), Exp.Const (Const.Cfun procname), [(Lvar pvar, typ)], _, _)
+        when QualifiedCppName.Match.match_qualifiers std_string_c_str_matcher
+               (Procname.get_qualifiers procname) -> (
+        match typ.desc with
+        | Tptr (inside_typ, _) ->
+            if is_local pvar proc_desc then Domain.set_internal_pointer var inside_typ astate
+            else astate
+        | _ ->
+            astate )
+      | Call ((var, _), Exp.Const (Const.Cfun procname), (_, typ) :: _args, _, _) ->
+          let procname_qualifiers = Procname.get_qualifiers procname in
           if
-            QualifiedCppName.Match.match_qualifiers std_string_c_str_matcher
-              (Procname.get_qualifiers procname)
+            QualifiedCppName.Match.match_qualifiers cstring_using_encoding procname_qualifiers
+            || QualifiedCppName.Match.match_qualifiers utf8string procname_qualifiers
           then
             match typ.desc with
             | Tptr (inside_typ, _) ->
-                Domain.set_internal_pointer var pvar inside_typ proc_desc astate
+                Domain.set_internal_pointer var inside_typ astate
             | _ ->
                 astate
           else astate

infer/src/checkers/SelfInBlock.ml

@@ -692,7 +692,7 @@ let report_self_in_block_passed_to_init_issue proc_desc err_log domain (captured
 
 let noescaping_matcher = QualifiedCppName.Match.of_fuzzy_qual_names Config.noescaping_function_list
 
-let should_ignore_cxx_captured attributes =
+let should_ignore_captured attributes =
   match attributes.ProcAttributes.block_as_arg_attributes with
   | Some {passed_to; passed_as_noescape_block} ->
       passed_as_noescape_block
@@ -712,7 +712,7 @@ let init_trace_captured_var data captured_definition_loc =
 let report_cxx_ref_captured_in_block proc_desc err_log domain (cxx_ref : DomainData.t)
     captured_definition_loc =
   let attributes = Procdesc.get_attributes proc_desc in
-  if not (should_ignore_cxx_captured attributes) then
+  if not (should_ignore_captured attributes) then
     let message =
       F.asprintf
         "The variable `%a` is a C++ reference and it's captured in the block. This can lead to \
@@ -728,6 +728,10 @@ let report_cxx_ref_captured_in_block proc_desc err_log domain (cxx_ref : DomainD
 
 let std_string_matcher = QualifiedCppName.Match.of_fuzzy_qual_names ["std::basic_string"]
 
+let std_string = "std::string"
+
+let nsstring = "NSString"
+
 let is_std_string typ =
   match typ.Typ.desc with
   | Tstruct name ->
@@ -737,20 +741,35 @@ let is_std_string typ =
       false
 
 
-let report_internal_pointer_captured_in_block proc_desc err_log domain (cxx_string : DomainData.t)
+let is_nsstring typ =
+  match typ.Typ.desc with
+  | Tstruct name ->
+      let name_s = Typ.Name.name name in
+      String.equal nsstring name_s
+  | _ ->
+      false
+
+
+let report_internal_pointer_captured_in_block proc_desc err_log domain (string : DomainData.t)
     typ_string captured_definition_loc =
   let attributes = Procdesc.get_attributes proc_desc in
-  if not (should_ignore_cxx_captured attributes) then
+  if not (should_ignore_captured attributes) then
     let message =
       F.asprintf
         "The local variable `%a` is a pointer internal to a `%s` object and it's captured in the \
          block. This can lead to crashes if the object is freed before the block's execution."
-        (Pvar.pp Pp.text) cxx_string.pvar typ_string
+        (Pvar.pp Pp.text) string.pvar typ_string
     in
-    let init_trace_elem = init_trace_captured_var cxx_string captured_definition_loc in
-    let ltr = init_trace_elem :: make_trace_captured domain cxx_string.pvar in
-    Reporting.log_issue proc_desc err_log ~ltr ~loc:cxx_string.loc SelfInBlock
-      IssueType.cxx_string_captured_in_block message
+    let init_trace_elem = init_trace_captured_var string captured_definition_loc in
+    let ltr = init_trace_elem :: make_trace_captured domain string.pvar in
+    let issue_type, suggestion =
+      if String.equal typ_string std_string then (IssueType.cxx_string_captured_in_block, None)
+      else
+        ( IssueType.ns_string_captured_in_block
+        , Some "You should copy the C string before using it in the block." )
+    in
+    Reporting.log_issue proc_desc err_log ~ltr ?suggestion ~loc:string.loc SelfInBlock issue_type
+      message
   else ()
 
 
@@ -764,9 +783,13 @@ let report_issues proc_desc err_log domain =
         report_cxx_ref_captured_in_block proc_desc err_log domain domain_data
           captured_definition_loc ;
         result
-    | DomainData.INTERNAL_POINTER (typ, captured_definition_loc) when is_std_string typ ->
-        report_internal_pointer_captured_in_block proc_desc err_log domain domain_data "std::string"
-          captured_definition_loc ;
+    | DomainData.INTERNAL_POINTER (typ, captured_definition_loc) ->
+        if is_std_string typ then
+          report_internal_pointer_captured_in_block proc_desc err_log domain domain_data std_string
+            captured_definition_loc ;
+        if is_nsstring typ then
+          report_internal_pointer_captured_in_block proc_desc err_log domain domain_data nsstring
+            captured_definition_loc ;
         result
     | DomainData.WEAK_SELF ->
         let _ =

infer/tests/codetoanalyze/objc/self-in-block/CStringUsingEncodingTest.m

@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <Foundation/Foundation.h>
+
+@interface CStringUsingEncodingTest : NSObject
+
+@end
+
+@implementation CStringUsingEncodingTest
+
+- (void)cStringUsingEncoding_no_copy_bad:(NSString*)name {
+  char* encoding = (char*)[name cStringUsingEncoding:NSUTF8StringEncoding];
+  dispatch_async(dispatch_get_main_queue(), ^{
+    const char* c = encoding;
+  });
+}
+
+- (void)cStringUsingEncoding_copy_good:(NSString*)name {
+  char* encoding = (char*)[name cStringUsingEncoding:NSUTF8StringEncoding];
+  char* encoding_copy = "";
+  strcpy(encoding_copy, encoding);
+  dispatch_async(dispatch_get_main_queue(), ^{
+    const char* c = encoding_copy;
+  });
+}
+
+- (void)UTF8String_no_copy_bad:(NSString*)name {
+  char* utf8string = name.UTF8String;
+  dispatch_async(dispatch_get_main_queue(), ^{
+    const char* c = utf8string;
+  });
+}
+
+@end

infer/tests/codetoanalyze/objc/self-in-block/issues.exp

@@ -1,3 +1,5 @@
+codetoanalyze/objc/self-in-block/CStringUsingEncodingTest.m, objc_block_CStringUsingEncodingTest.m:18, 1, NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &encoding defined here,Using captured &encoding]
+codetoanalyze/objc/self-in-block/CStringUsingEncodingTest.m, objc_block_CStringUsingEncodingTest.m:34, 1, NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &utf8string defined here,Using captured &utf8string]
 codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_block_NoescapeBlock.m:35, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf]
 codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_block_NoescapeBlock.m:35, 1, WEAK_SELF_IN_NO_ESCAPE_BLOCK, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf]
 codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_block_NoescapeBlock.m:35, 2, WEAK_SELF_IN_NO_ESCAPE_BLOCK, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf]