From 2886e849dac681a157e8207da1a12e31b0174a71 Mon Sep 17 00:00:00 2001 From: Sungkeun Cho Date: Mon, 26 Apr 2021 02:35:19 -0700 Subject: [PATCH] [frontend,pulse] Avoid dereference of C struct Summary: This diff avoids dereference of C struct, in its frontend and its semantics of Pulse. In SIL, C struct is not first-class value, thus dereferencing on it does not make sense. Reviewed By: ezgicicek Differential Revision: D27953258 fbshipit-source-id: 348d56338 --- infer/src/clang/cTrans_utils.ml | 15 ++++++++---- infer/src/pulse/PulseCallOperations.ml | 4 ++-- infer/src/pulse/PulseInterproc.ml | 24 +++++++++++++------ infer/src/pulse/PulseInterproc.mli | 2 +- infer/src/pulse/PulseOperations.ml | 4 ++-- infer/src/pulse/PulseOperations.mli | 4 ++-- .../tests/codetoanalyze/cpp/pulse/issues.exp | 1 - .../codetoanalyze/cpp/pulse/optional.cpp | 3 ++- .../frontend/property/Property_getter.m.dot | 2 +- .../tests/codetoanalyze/objc/pulse/issues.exp | 1 + infer/tests/codetoanalyze/objc/pulse/uninit.m | 5 ++-- 11 files changed, 40 insertions(+), 25 deletions(-) diff --git a/infer/src/clang/cTrans_utils.ml b/infer/src/clang/cTrans_utils.ml index 78fc82140..9cb7ee3d0 100644 --- a/infer/src/clang/cTrans_utils.ml +++ b/infer/src/clang/cTrans_utils.ml @@ -560,11 +560,16 @@ let cast_operation ?objc_bridge_cast_kind cast_kind ((exp, typ) as exp_typ) cast | `BitCast | `IntegralCast | `IntegralToBoolean -> (* This is treated as a nop by returning the same expressions exps*) ([], (exp, cast_typ)) - | `LValueToRValue -> - (* Takes an LValue and allow it to use it as RValue. *) - (* So we assign the LValue to a temp and we pass it to the parent.*) - let instr, deref_exp = dereference_var_sil (exp, cast_typ) sil_loc in - ([instr], (deref_exp, cast_typ)) + | `LValueToRValue -> ( + match typ with + | {Typ.desc= Tstruct _} -> + (* Avoid dereference of C struct on cast *) + ([], exp_typ) + | _ -> + (* Takes an LValue and allow it to use it as RValue. *) + (* So we assign the LValue to a temp and we pass it to the parent.*) + let instr, deref_exp = dereference_var_sil (exp, cast_typ) sil_loc in + ([instr], (deref_exp, cast_typ)) ) | `NullToPointer -> if Exp.is_zero exp then ([], (Exp.null, cast_typ)) else ([], (exp, cast_typ)) | `ToVoid -> diff --git a/infer/src/pulse/PulseCallOperations.ml b/infer/src/pulse/PulseCallOperations.ml index 54c6b242e..c9b400c8f 100644 --- a/infer/src/pulse/PulseCallOperations.ml +++ b/infer/src/pulse/PulseCallOperations.ml @@ -198,9 +198,9 @@ let call_aux tenv caller_proc_desc call_loc callee_pname ret actuals callee_proc in let captured_vars = Procdesc.get_captured callee_proc_desc - |> List.map ~f:(fun {CapturedVar.name; capture_mode} -> + |> List.map ~f:(fun {CapturedVar.name; capture_mode; typ} -> let pvar = Pvar.mk name callee_pname in - (Var.of_pvar pvar, capture_mode) ) + (Var.of_pvar pvar, capture_mode, typ) ) in let<*> astate, captured_vars_with_actuals = match actuals with diff --git a/infer/src/pulse/PulseInterproc.ml b/infer/src/pulse/PulseInterproc.ml index 736abe8dd..1480fa7bd 100644 --- a/infer/src/pulse/PulseInterproc.ml +++ b/infer/src/pulse/PulseInterproc.ml @@ -205,13 +205,22 @@ let rec materialize_pre_from_address callee_proc_name call_location ~pre ~addr_p ~addr_hist_caller:addr_hist_dest_caller call_state ) ) +let deref_non_c_struct addr typ astate = + match typ.Typ.desc with + | Tstruct _ -> + Some addr + | _ -> + BaseMemory.find_edge_opt addr Dereference astate |> Option.map ~f:fst + + (** materialize subgraph of [pre] rooted at the address represented by a [formal] parameter that has been instantiated with the corresponding [actual] into the current state [call_state.astate] *) -let materialize_pre_from_actual callee_proc_name call_location ~pre ~formal ~actual call_state = +let materialize_pre_from_actual callee_proc_name call_location ~pre ~formal ~actual:(actual, typ) + call_state = L.d_printfln "Materializing PRE from [%a <- %a]" Var.pp formal AbstractValue.pp (fst actual) ; (let open IOption.Let_syntax in let* addr_formal_pre, _ = BaseStack.find_opt formal pre.BaseDomain.stack in - let+ formal_pre, _ = BaseMemory.find_edge_opt addr_formal_pre Dereference pre.BaseDomain.heap in + let+ formal_pre = deref_non_c_struct addr_formal_pre typ pre.BaseDomain.heap in materialize_pre_from_address callee_proc_name call_location ~pre ~addr_pre:formal_pre ~addr_hist_caller:actual call_state) |> function Some result -> result | None -> Ok call_state @@ -236,7 +245,7 @@ let materialize_pre_for_parameters callee_proc_name call_location pre_post ~form call [materialize_pre_from] on them. Give up if calling the function introduces aliasing. *) match - IList.fold2_result formals actuals ~init:call_state ~f:(fun call_state formal (actual, _) -> + IList.fold2_result formals actuals ~init:call_state ~f:(fun call_state formal actual -> materialize_pre_from_actual callee_proc_name call_location ~pre:(pre_post.AbductiveDomain.pre :> BaseDomain.t) ~formal ~actual call_state ) @@ -468,15 +477,16 @@ let rec record_post_for_address callee_proc_name call_loc ({AbductiveDomain.pre; ~addr_hist_caller:addr_hist_curr_dest call_state ) ) -let record_post_for_actual callee_proc_name call_loc pre_post ~formal ~actual call_state = +let record_post_for_actual callee_proc_name call_loc pre_post ~formal ~actual:(actual, typ) + call_state = L.d_printfln_escaped "Recording POST from [%a] <-> %a" Var.pp formal AbstractValue.pp (fst actual) ; match let open IOption.Let_syntax in let* addr_formal_pre, _ = BaseStack.find_opt formal (pre_post.AbductiveDomain.pre :> BaseDomain.t).BaseDomain.stack in - let+ formal_pre, _ = - BaseMemory.find_edge_opt addr_formal_pre Dereference + let+ formal_pre = + deref_non_c_struct addr_formal_pre typ (pre_post.AbductiveDomain.pre :> BaseDomain.t).BaseDomain.heap in record_post_for_address callee_proc_name call_loc pre_post ~addr_callee:formal_pre @@ -525,7 +535,7 @@ let apply_post_for_parameters callee_proc_name call_location pre_post ~formals ~ between pre and post since it's unreliable, eg replace value read in pre with same value in post but nuke other fields in the meantime? is that possible?). *) match - List.fold2 formals actuals ~init:call_state ~f:(fun call_state formal (actual, _) -> + List.fold2 formals actuals ~init:call_state ~f:(fun call_state formal actual -> record_post_for_actual callee_proc_name call_location pre_post ~formal ~actual call_state ) with | Unequal_lengths -> diff --git a/infer/src/pulse/PulseInterproc.mli b/infer/src/pulse/PulseInterproc.mli index 763f17da2..8af5e7076 100644 --- a/infer/src/pulse/PulseInterproc.mli +++ b/infer/src/pulse/PulseInterproc.mli @@ -14,7 +14,7 @@ val apply_prepost : -> Procname.t -> Location.t -> callee_prepost:AbductiveDomain.t - -> captured_vars_with_actuals:(Var.t * (AbstractValue.t * ValueHistory.t)) list + -> captured_vars_with_actuals:(Var.t * ((AbstractValue.t * ValueHistory.t) * Typ.t)) list -> formals:Var.t list -> actuals:((AbstractValue.t * ValueHistory.t) * Typ.t) list -> AbductiveDomain.t diff --git a/infer/src/pulse/PulseOperations.ml b/infer/src/pulse/PulseOperations.ml index 06081e153..04c10a58f 100644 --- a/infer/src/pulse/PulseOperations.ml +++ b/infer/src/pulse/PulseOperations.ml @@ -561,12 +561,12 @@ let get_captured_actuals location ~captured_vars ~actual_closure astate = let* astate, this_value_addr = eval_access Read location actual_closure Dereference astate in let+ _, astate, captured_vars_with_actuals = List.fold_result captured_vars ~init:(0, astate, []) - ~f:(fun (id, astate, captured) (var, mode) -> + ~f:(fun (id, astate, captured) (var, mode, typ) -> let+ astate, captured_actual = eval_access Read location this_value_addr (FieldAccess (Closures.mk_fake_field ~id mode)) astate in - (id + 1, astate, (var, captured_actual) :: captured) ) + (id + 1, astate, (var, (captured_actual, typ)) :: captured) ) in (astate, captured_vars_with_actuals) diff --git a/infer/src/pulse/PulseOperations.mli b/infer/src/pulse/PulseOperations.mli index 5b01ff1aa..c5e1655b6 100644 --- a/infer/src/pulse/PulseOperations.mli +++ b/infer/src/pulse/PulseOperations.mli @@ -204,7 +204,7 @@ val check_address_escape : val get_captured_actuals : Location.t - -> captured_vars:(Var.t * Pvar.capture_mode) list + -> captured_vars:(Var.t * Pvar.capture_mode * Typ.t) list -> actual_closure:AbstractValue.t * ValueHistory.t -> t - -> (t * (Var.t * (AbstractValue.t * ValueHistory.t)) list) AccessResult.t + -> (t * (Var.t * ((AbstractValue.t * ValueHistory.t) * Typ.t)) list) AccessResult.t diff --git a/infer/tests/codetoanalyze/cpp/pulse/issues.exp b/infer/tests/codetoanalyze/cpp/pulse/issues.exp index 6d0c9b1a6..1aeda2a31 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/issues.exp +++ b/infer/tests/codetoanalyze/cpp/pulse/issues.exp @@ -70,7 +70,6 @@ codetoanalyze/cpp/pulse/optional.cpp, inside_try_catch_FP, 3, OPTIONAL_EMPTY_ACC codetoanalyze/cpp/pulse/optional.cpp, none_copy_bad, 3, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),passed as argument to `folly::Optional::Optional(folly::Optional arg)` (modelled),return from call to `folly::Optional::Optional(folly::Optional arg)` (modelled),invalid access occurs here] codetoanalyze/cpp/pulse/optional.cpp, none_no_check_bad, 2, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),invalid access occurs here] codetoanalyze/cpp/pulse/optional.cpp, not_none_check_value_ok_FP, 5, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),invalid access occurs here] -codetoanalyze/cpp/pulse/optional.cpp, operator_arrow_bad, 0, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),when calling `emplace` here,parameter `state` of emplace,passed as argument to `folly::Optional::operator->()` (modelled),return from call to `folly::Optional::operator->()` (modelled),invalid access occurs here] codetoanalyze/cpp/pulse/optional.cpp, std_assign2_bad, 4, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `std::optional::operator=(None)` (modelled),return from call to `std::optional::operator=(None)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `std::optional::operator=(None)` (modelled),return from call to `std::optional::operator=(None)` (modelled),invalid access occurs here] codetoanalyze/cpp/pulse/optional.cpp, std_assign_bad, 5, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),passed as argument to `std::optional::operator=(std::optional arg)` (modelled),return from call to `std::optional::operator=(std::optional arg)` (modelled),invalid access occurs here] codetoanalyze/cpp/pulse/optional.cpp, std_none_copy_bad, 3, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),passed as argument to `std::optional::optional(std::optional arg)` (modelled),return from call to `std::optional::optional(std::optional arg)` (modelled),invalid access occurs here] diff --git a/infer/tests/codetoanalyze/cpp/pulse/optional.cpp b/infer/tests/codetoanalyze/cpp/pulse/optional.cpp index fd64fb62a..8299405d7 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/optional.cpp +++ b/infer/tests/codetoanalyze/cpp/pulse/optional.cpp @@ -148,7 +148,8 @@ void emplace(folly::Optional state) { auto pos = state->vec.begin(); } -void operator_arrow_bad() { emplace(folly::none); } +/* There is a bug in the frontend T89443328 */ +void operator_arrow_bad_FN() { emplace(folly::none); } void get_pointer_check_none_check_ok() { folly::Optional foo{folly::none}; diff --git a/infer/tests/codetoanalyze/objc/frontend/property/Property_getter.m.dot b/infer/tests/codetoanalyze/objc/frontend/property/Property_getter.m.dot index ecd982df5..78fd7bb38 100644 --- a/infer/tests/codetoanalyze/objc/frontend/property/Property_getter.m.dot +++ b/infer/tests/codetoanalyze/objc/frontend/property/Property_getter.m.dot @@ -15,7 +15,7 @@ digraph cfg { "getMyStructField:my_struct:#A(class A)#class.35da25720e80922866aa41dc70b313cb_4" -> "getMyStructField:my_struct:#A(class A)#class.35da25720e80922866aa41dc70b313cb_2" ; -"getMyStructField:my_struct:#A(class A)#class.35da25720e80922866aa41dc70b313cb_5" [label="5: Message Call: setS: \n n$8=*&target:A* [line 27, column 3]\n n$7=*&my_struct:my_struct [line 27, column 14]\n n$9=_fun_A.setS:(n$8:A*,n$7:my_struct) [line 27, column 10]\n " shape="box"] +"getMyStructField:my_struct:#A(class A)#class.35da25720e80922866aa41dc70b313cb_5" [label="5: Message Call: setS: \n n$7=*&target:A* [line 27, column 3]\n n$8=_fun_A.setS:(n$7:A*,&my_struct:my_struct) [line 27, column 10]\n " shape="box"] "getMyStructField:my_struct:#A(class A)#class.35da25720e80922866aa41dc70b313cb_5" -> "getMyStructField:my_struct:#A(class A)#class.35da25720e80922866aa41dc70b313cb_3" ; diff --git a/infer/tests/codetoanalyze/objc/pulse/issues.exp b/infer/tests/codetoanalyze/objc/pulse/issues.exp index 113b6b3e7..b7de9a709 100644 --- a/infer/tests/codetoanalyze/objc/pulse/issues.exp +++ b/infer/tests/codetoanalyze/objc/pulse/issues.exp @@ -9,5 +9,6 @@ codetoanalyze/objc/pulse/MemoryLeaks.m, call_cfrelease_interproc_leak_ok_FP, 2, codetoanalyze/objc/pulse/MemoryLeaksInBlocks.m, block_captured_var_leak_bad, 6, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by call to `malloc_no_fail` (modelled),allocation part of the trace ends here,memory becomes unreachable here] codetoanalyze/objc/pulse/NPEBlocks.m, Singleton.dispatch_once_no_npe_good_FP, 6, NULLPTR_DEREFERENCE, no_bucket, ERROR, [source of the null value part of the trace starts here,assigned,is the null pointer,null pointer dereference part of the trace starts here,assigned,invalid access occurs here] codetoanalyze/objc/pulse/NPEBlocks.m, captured_npe_bad, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [source of the null value part of the trace starts here,assigned,is the null pointer,null pointer dereference part of the trace starts here,assigned,when calling `objc_blockcaptured_npe_bad_3` here,parameter `x` of objc_blockcaptured_npe_bad_3,invalid access occurs here] +codetoanalyze/objc/pulse/uninit.m, Uninit.call_setter_c_struct_bad, 3, PULSE_UNINITIALIZED_VALUE, no_bucket, ERROR, [struct field address `x` created,when calling `Uninit.setS:` here,parameter `s` of Uninit.setS:,read to uninitialized value occurs here] codetoanalyze/objc/pulse/use_after_free.m, PulseTest.use_after_free_simple_in_objc_method_bad:, 2, USE_AFTER_FREE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `x` of PulseTest.use_after_free_simple_in_objc_method_bad:,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of PulseTest.use_after_free_simple_in_objc_method_bad:,invalid access occurs here] codetoanalyze/objc/pulse/use_after_free.m, use_after_free_simple_bad, 2, USE_AFTER_FREE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `x` of use_after_free_simple_bad,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of use_after_free_simple_bad,invalid access occurs here] diff --git a/infer/tests/codetoanalyze/objc/pulse/uninit.m b/infer/tests/codetoanalyze/objc/pulse/uninit.m index 20874e108..59d112f22 100644 --- a/infer/tests/codetoanalyze/objc/pulse/uninit.m +++ b/infer/tests/codetoanalyze/objc/pulse/uninit.m @@ -130,10 +130,9 @@ dispatch_queue_t queue; return [Uninit getter_c_struct:obj]; } -/* FN due to the frontend is incorrect when passing C struct value as a function - paraemeter when calling C struct setter. */ -+ (void)call_setter_c_struct_bad_FN:(Uninit*)obj { ++ (void)call_setter_c_struct_bad { struct my_struct s; + Uninit* obj = [Uninit new]; obj.s = s; // setter is called }