From 051473394b26fc39c08c90bf10f0a529e7a8b878 Mon Sep 17 00:00:00 2001 From: Sungkeun Cho Date: Wed, 27 Jan 2021 10:44:05 -0800 Subject: [PATCH] [frontend] Fix incorrect order of statements Summary: This diff fixes incorrect order of statements on assignments. In the translation of `LHS=RHS;`, if `RHS` is a complicated expression that introduced new nodes, eg a conditional expression, some load statements for `LHS` came after its usage. To avoid the issue, this diff forces it to introduce new nodes for `LHS`. Reviewed By: jvillard Differential Revision: D26099782 fbshipit-source-id: 27417cd99 --- infer/src/clang/cTrans.ml | 14 +++- .../codetoanalyze/cpp/pulse/frontend.cpp | 76 +++++++++++++++++++ .../tests/codetoanalyze/cpp/pulse/issues.exp | 3 + .../codetoanalyze/cpp/pulse/temporaries.cpp | 2 +- 4 files changed, 93 insertions(+), 2 deletions(-) diff --git a/infer/src/clang/cTrans.ml b/infer/src/clang/cTrans.ml index 3215986fe..ffbc7306e 100644 --- a/infer/src/clang/cTrans.ml +++ b/infer/src/clang/cTrans.ml @@ -1089,7 +1089,19 @@ module CTrans_funct (F : CModule_type.CFrontend) : CModule_type.CTranslation = s let binop_control = {empty_control with instrs= instr_bin @ extra_instrs} in ([binop_control], return) in - let all_res_trans = [res_trans_e1.control; res_trans_e2.control] @ binop_control in + (* In the translation of [LHS=RHS;], if [RHS] is a complicated expression that introduced + new nodes, eg a conditional expression, it forces to introduce nodes for [LHS], in order + to avoid an incorrect statement order. *) + let e1_control = + match (binary_operator_info.Clang_ast_t.boi_kind, s2) with + | `Assign, Clang_ast_t.ConditionalOperator _ -> + let trans_state' = PriorityNode.try_claim_priority_node trans_state' stmt_info in + PriorityNode.compute_controls_to_parent trans_state' sil_loc node_name stmt_info + [res_trans_e1.control] + | _, _ -> + res_trans_e1.control + in + let all_res_trans = [e1_control; res_trans_e2.control] @ binop_control in PriorityNode.compute_controls_to_parent trans_state_pri sil_loc node_name stmt_info all_res_trans |> mk_trans_result return diff --git a/infer/tests/codetoanalyze/cpp/pulse/frontend.cpp b/infer/tests/codetoanalyze/cpp/pulse/frontend.cpp index 6e7d563b6..f4396d75f 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/frontend.cpp +++ b/infer/tests/codetoanalyze/cpp/pulse/frontend.cpp @@ -99,4 +99,80 @@ int conditional_expression_bad(bool b) { } } +class Frontend { + public: + int a; + + Frontend(int n) { a = (n == 0) ? 0 : 1; } + + Frontend(Frontend x, int n) { + x.a = (n == 0) ? 0 : 1; + a = x.a; + } + + void set_field_via_local(int n) { + int* b = (int*)malloc(sizeof(int)); + if (b) { + *b = 0; + *b = (n == 0) ? 0 : 1; + a = *b; + free(b); + } else { + a = (n == 0) ? 0 : 1; + } + } +}; + +void call_Frontend_constructor_ok() { + Frontend x = Frontend(10); // x.a is 1 + if (x.a != 1) { + int* p = nullptr; + *p = 42; + } +} + +void call_Frontend_constructor_bad() { + Frontend x = Frontend(10); // x.a is 1 + if (x.a == 1) { + int* p = nullptr; + *p = 42; + } +} + +void call_Frontend_constructor2_ok() { + Frontend x = Frontend(0); // x.a is 0 + Frontend y = Frontend(x, 10); // y.a is 1 + if (y.a != 1) { + int* p = nullptr; + *p = 42; + } +} + +void call_Frontend_constructor2_bad() { + Frontend x = Frontend(0); // x.a is 0 + Frontend y = Frontend(x, 10); // y.a is 1 + if (y.a == 1) { + int* p = nullptr; + *p = 42; + } +} + +void call_set_field_via_local_ok() { + Frontend x = Frontend(0); // x.a is 0 + x.set_field_via_local(10); // x.a is 1 + if (x.a != 1) { + int* p = nullptr; + *p = 42; + } +} + +void call_set_field_via_local_bad() { + Frontend x = Frontend(0); // x.a is 0 + x.set_field_via_local(10); // x.a is 1 + if (x.a == 1) { + int* p = nullptr; + *p = 42; + } +} + } // namespace frontend diff --git a/infer/tests/codetoanalyze/cpp/pulse/issues.exp b/infer/tests/codetoanalyze/cpp/pulse/issues.exp index 630ede5be..9031b7af4 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/issues.exp +++ b/infer/tests/codetoanalyze/cpp/pulse/issues.exp @@ -32,6 +32,9 @@ codetoanalyze/cpp/pulse/deduplication.cpp, deduplication::templated_function_bad codetoanalyze/cpp/pulse/deduplication.cpp, deduplication::templated_function_bad, 3, USE_AFTER_DELETE, no_bucket, ERROR, [invalidation part of the trace starts here,passed as argument to `new` (modelled),return from call to `new` (modelled),assigned,when calling `deduplication::templated_delete_function` here,parameter `a` of deduplication::templated_delete_function,was invalidated by `delete`,use-after-lifetime part of the trace starts here,passed as argument to `new` (modelled),return from call to `new` (modelled),assigned,when calling `deduplication::templated_access_function` here,parameter `a` of deduplication::templated_access_function,invalid access occurs here] codetoanalyze/cpp/pulse/exit_test.cpp, store_exit_null_bad, 0, NULLPTR_DEREFERENCE, no_bucket, ERROR, [invalidation part of the trace starts here,assigned,is the null pointer,use-after-lifetime part of the trace starts here,when calling `store_exit` here,parameter `x` of store_exit,invalid access occurs here] codetoanalyze/cpp/pulse/folly_DestructorGuard.cpp, UsingDelayedDestruction::double_delete_bad, 2, USE_AFTER_DELETE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `this` of UsingDelayedDestruction::double_delete_bad,was invalidated by `delete`,use-after-lifetime part of the trace starts here,parameter `this` of UsingDelayedDestruction::double_delete_bad,invalid access occurs here] +codetoanalyze/cpp/pulse/frontend.cpp, frontend::call_Frontend_constructor2_bad, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [invalidation part of the trace starts here,assigned,is the null pointer,use-after-lifetime part of the trace starts here,assigned,invalid access occurs here] +codetoanalyze/cpp/pulse/frontend.cpp, frontend::call_Frontend_constructor_bad, 4, NULLPTR_DEREFERENCE, no_bucket, ERROR, [invalidation part of the trace starts here,assigned,is the null pointer,use-after-lifetime part of the trace starts here,assigned,invalid access occurs here] +codetoanalyze/cpp/pulse/frontend.cpp, frontend::call_set_field_via_local_bad, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [invalidation part of the trace starts here,assigned,is the null pointer,use-after-lifetime part of the trace starts here,assigned,invalid access occurs here] codetoanalyze/cpp/pulse/frontend.cpp, frontend::conditional_expression_bad, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [invalidation part of the trace starts here,assigned,is the null pointer,use-after-lifetime part of the trace starts here,assigned,invalid access occurs here] codetoanalyze/cpp/pulse/frontend.cpp, frontend::deref_null_namespace_alias_ptr_bad, 4, USE_AFTER_DELETE, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `frontend::some::thing::bad_ptr` here,passed as argument to `new` (modelled),return from call to `new` (modelled),assigned,was invalidated by `delete`,use-after-lifetime part of the trace starts here,passed as argument to `frontend::some::thing::bad_ptr`,return from call to `frontend::some::thing::bad_ptr`,assigned,invalid access occurs here] codetoanalyze/cpp/pulse/interprocedural.cpp, access_to_invalidated_alias2_bad, 3, USE_AFTER_DELETE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `x` of access_to_invalidated_alias2_bad,assigned,when calling `invalidate_and_set_to_null` here,parameter `x_ptr` of invalidate_and_set_to_null,was invalidated by `delete`,use-after-lifetime part of the trace starts here,parameter `x` of access_to_invalidated_alias2_bad,when calling `wraps_read` here,parameter `x` of wraps_read,when calling `wraps_read_inner` here,parameter `x` of wraps_read_inner,invalid access occurs here] diff --git a/infer/tests/codetoanalyze/cpp/pulse/temporaries.cpp b/infer/tests/codetoanalyze/cpp/pulse/temporaries.cpp index c2ff5e87d..814aa4fe2 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/temporaries.cpp +++ b/infer/tests/codetoanalyze/cpp/pulse/temporaries.cpp @@ -32,7 +32,7 @@ struct UniquePtr { struct A { int s_; ~A() {} - A() { A(42); } + A() : A(42) {} A(int s) { s_ = s; } A(A& a) { s_ = a.s_; } };