From 2c48e61031be9eaed877daf512f7dfcbe4453840 Mon Sep 17 00:00:00 2001 From: Daiva Naudziuniene Date: Tue, 30 Jun 2020 01:55:14 -0700 Subject: [PATCH] [pulse] A new issue type OPTIONAL_EMPTY_ACCESS for trying to access folly::Optional when it is folly::none Summary: We need to check if `folly::Optional` is not `folly::none` if we want to retrieve the value, otherwise a runtime exception is thrown: ``` folly::Optional foo{folly::none}; return foo.value(); // bad ``` ``` folly::Optional foo{folly::none}; if (foo) { return foo.value(); // ok } ``` This diff adds a new issue type that reports if we try to access `folly::Optional` value when it is known to be `folly::none`. Reviewed By: ezgicicek Differential Revision: D22053352 fbshipit-source-id: 32cb00a99 --- infer/man/man1/infer-full.txt | 1 + infer/man/man1/infer-report.txt | 1 + infer/man/man1/infer.txt | 1 + infer/src/base/IssueType.ml | 5 + infer/src/base/IssueType.mli | 2 + infer/src/pulse/PulseInvalidation.ml | 7 +- infer/src/pulse/PulseInvalidation.mli | 1 + infer/src/pulse/PulseModels.ml | 96 ++++++++++++- .../tests/codetoanalyze/cpp/pulse/issues.exp | 5 + .../codetoanalyze/cpp/pulse/optional.cpp | 130 ++++++++++++++++++ 10 files changed, 247 insertions(+), 2 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/pulse/optional.cpp diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 8f50b25fa..81244270e 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -461,6 +461,7 @@ OPTIONS Missing_fld (enabled by default), NULLPTR_DEREFERENCE (disabled by default), NULL_DEREFERENCE (enabled by default), + OPTIONAL_EMPTY_ACCESS (disabled by default), PARAMETER_NOT_NULL_CHECKED (enabled by default), POINTER_TO_CONST_OBJC_CLASS (enabled by default), PRECONDITION_NOT_FOUND (enabled by default), diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index 4173d4c6a..f16e2b5e9 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -191,6 +191,7 @@ OPTIONS Missing_fld (enabled by default), NULLPTR_DEREFERENCE (disabled by default), NULL_DEREFERENCE (enabled by default), + OPTIONAL_EMPTY_ACCESS (disabled by default), PARAMETER_NOT_NULL_CHECKED (enabled by default), POINTER_TO_CONST_OBJC_CLASS (enabled by default), PRECONDITION_NOT_FOUND (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index e5d24ed04..54c619ef7 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -461,6 +461,7 @@ OPTIONS Missing_fld (enabled by default), NULLPTR_DEREFERENCE (disabled by default), NULL_DEREFERENCE (enabled by default), + OPTIONAL_EMPTY_ACCESS (disabled by default), PARAMETER_NOT_NULL_CHECKED (enabled by default), POINTER_TO_CONST_OBJC_CLASS (enabled by default), PRECONDITION_NOT_FOUND (enabled by default), diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index bcea79b63..2f5a5caf9 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -745,6 +745,11 @@ let nullptr_dereference = ~user_documentation:"See [NULL_DEREFERENCE](#null_dereference)." +let optional_empty_access = + register ~enabled:false ~id:"OPTIONAL_EMPTY_ACCESS" Error Pulse + ~user_documentation:"Reports on accessing folly::Optional when it is none." + + let parameter_not_null_checked = register ~id:"PARAMETER_NOT_NULL_CHECKED" Warning Biabduction ~user_documentation:[%blob "../../documentation/issues/PARAMETER_NOT_NULL_CHECKED.md"] diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 145efca64..ac17c9651 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -264,6 +264,8 @@ val null_dereference : t val nullptr_dereference : t +val optional_empty_access : t + val parameter_not_null_checked : t val precondition_not_found : t diff --git a/infer/src/pulse/PulseInvalidation.ml b/infer/src/pulse/PulseInvalidation.ml index bbc2ec40b..114d6e918 100644 --- a/infer/src/pulse/PulseInvalidation.ml +++ b/infer/src/pulse/PulseInvalidation.ml @@ -47,6 +47,7 @@ type t = | CppDelete | EndIterator | GoneOutOfScope of Pvar.t * Typ.t + | OptionalEmpty | StdVector of std_vector_function | JavaIterator of java_iterator_function [@@deriving compare] @@ -64,6 +65,8 @@ let issue_type_of_cause = function IssueType.vector_invalidation | GoneOutOfScope _ -> IssueType.use_after_lifetime + | OptionalEmpty -> + IssueType.optional_empty_access | JavaIterator _ | StdVector _ -> IssueType.vector_invalidation @@ -87,6 +90,8 @@ let describe f cause = else F.fprintf f "is the address of a stack variable `%a`" Pvar.pp_value pvar in F.fprintf f "%a whose lifetime has ended" pp_var pvar + | OptionalEmpty -> + F.pp_print_string f "is folly::None" | StdVector std_vector_f -> F.fprintf f "was potentially invalidated by `%a()`" pp_std_vector_function std_vector_f | JavaIterator java_iterator_f -> @@ -101,7 +106,7 @@ let pp f invalidation = F.fprintf f "ConstantDereference(%a)" describe invalidation | CppDelete -> F.fprintf f "CppDelete(%a)" describe invalidation - | EndIterator | GoneOutOfScope _ -> + | EndIterator | GoneOutOfScope _ | OptionalEmpty -> describe f invalidation | StdVector _ -> F.fprintf f "StdVector(%a)" describe invalidation diff --git a/infer/src/pulse/PulseInvalidation.mli b/infer/src/pulse/PulseInvalidation.mli index 1054a696c..e21065bc2 100644 --- a/infer/src/pulse/PulseInvalidation.mli +++ b/infer/src/pulse/PulseInvalidation.mli @@ -29,6 +29,7 @@ type t = | CppDelete | EndIterator | GoneOutOfScope of Pvar.t * Typ.t + | OptionalEmpty | StdVector of std_vector_function | JavaIterator of java_iterator_function [@@deriving compare] diff --git a/infer/src/pulse/PulseModels.ml b/infer/src/pulse/PulseModels.ml index 1384fc3c9..aaa56cf65 100644 --- a/infer/src/pulse/PulseModels.ml +++ b/infer/src/pulse/PulseModels.ml @@ -139,6 +139,82 @@ module ObjC = struct |> PulseOperations.ok_continue end +module FollyOptional = struct + let internal_value = + Fieldname.make + (Typ.CStruct (QualifiedCppName.of_list ["__infer_pulse_model"])) + "__infer_model_backing_value" + + + let internal_value_access = HilExp.Access.FieldAccess internal_value + + let to_internal_value location optional astate = + PulseOperations.eval_access location optional internal_value_access astate + + + let to_internal_value_deref location optional astate = + let* astate, pointer = to_internal_value location optional astate in + PulseOperations.eval_access location pointer Dereference astate + + + let write_value location this ~value ~desc astate = + let event = ValueHistory.Call {f= Model desc; location; in_call= []} in + let* astate, value_field = to_internal_value location this astate in + let value_hist = (fst value, event :: snd value) in + let+ astate = PulseOperations.write_deref location ~ref:value_field ~obj:value_hist astate in + (astate, value_hist) + + + let assign_value_fresh location this ~desc astate = + write_value location this ~value:(AbstractValue.mk_fresh (), []) ~desc astate + + + let assign_none this ~desc : model = + fun _ ~callee_procname:_ location ~ret:_ astate -> + let* astate, value = assign_value_fresh location this ~desc astate in + let astate = PulseArithmetic.and_eq_int (fst value) IntLit.zero astate in + let+ astate = PulseOperations.invalidate location Invalidation.OptionalEmpty value astate in + [ExecutionDomain.ContinueProgram astate] + + + let assign_value this init ~desc : model = + fun _ ~callee_procname:_ location ~ret:_ astate -> + let* astate, value = to_internal_value_deref location init astate in + let+ astate, _ = write_value location this ~value ~desc astate in + [ExecutionDomain.ContinueProgram astate] + + + let emplace optional : model = + fun _ ~callee_procname:_ location ~ret:_ astate -> + let+ astate, _ = + assign_value_fresh location optional ~desc:"folly::Optional::emplace()" astate + in + [ExecutionDomain.ContinueProgram astate] + + + let value optional : model = + fun _ ~callee_procname:_ location ~ret:(ret_id, _) astate -> + let event = ValueHistory.Call {f= Model "folly::Optional::value()"; location; in_call= []} in + let* astate, (value_addr, value_hist) = to_internal_value_deref location optional astate in + PulseOperations.write_id ret_id (value_addr, event :: value_hist) astate + |> PulseOperations.ok_continue + + + let has_value optional : model = + fun _ ~callee_procname:_ location ~ret:(ret_id, _) astate -> + let ret_addr = AbstractValue.mk_fresh () in + let ret_value = + (ret_addr, [ValueHistory.Allocation {f= Model "folly::Optional::has_value()"; location}]) + in + let+ astate, (value_addr, _) = to_internal_value_deref location optional astate in + let astate = PulseOperations.write_id ret_id ret_value astate in + let astate_non_empty = PulseArithmetic.and_positive value_addr astate in + let astate_true = PulseArithmetic.and_positive ret_addr astate_non_empty in + let astate_empty = PulseArithmetic.and_eq_int value_addr IntLit.zero astate in + let astate_false = PulseArithmetic.and_eq_int ret_addr IntLit.zero astate_empty in + [ExecutionDomain.ContinueProgram astate_false; ExecutionDomain.ContinueProgram astate_true] +end + module Cplusplus = struct let delete deleted_access : model = fun _ ~callee_procname:_ location ~ret:_ astate -> @@ -742,8 +818,26 @@ module ProcNameDispatcher = struct ref-counting *) -"folly" &:: "fbstring_core" &:: "category" &--> Misc.return_int Int64.zero ; -"folly" &:: "DelayedDestruction" &:: "destroy" &--> Misc.skip - ; -"folly" &:: "Optional" &:: "reset" &--> Misc.skip ; -"folly" &:: "SocketAddress" &:: "~SocketAddress" &--> Misc.skip + ; -"folly" &:: "Optional" &:: "Optional" <>$ capt_arg_payload + $+ any_arg_of_typ (-"folly" &:: "None") + $--> FollyOptional.assign_none ~desc:"folly::Optional::Optional(=None)" + ; -"folly" &:: "Optional" &:: "Optional" <>$ capt_arg_payload + $--> FollyOptional.assign_none ~desc:"folly::Optional::Optional()" + ; -"folly" &:: "Optional" &:: "Optional" <>$ capt_arg_payload $+ capt_arg_payload + $+...$--> FollyOptional.assign_value ~desc:"folly::Optional::Optional(arg)" + ; -"folly" &:: "Optional" &:: "assign" <>$ capt_arg_payload + $+ any_arg_of_typ (-"folly" &:: "None") + $--> FollyOptional.assign_none ~desc:"folly::Optional::assign(=None)" + ; -"folly" &:: "Optional" &:: "assign" <>$ capt_arg_payload $+ capt_arg_payload + $+...$--> FollyOptional.assign_value ~desc:"folly::Optional::assign()" + ; -"folly" &:: "Optional" &:: "emplace<>" $ capt_arg_payload $+...$--> FollyOptional.emplace + ; -"folly" &:: "Optional" &:: "emplace" $ capt_arg_payload $+...$--> FollyOptional.emplace + ; -"folly" &:: "Optional" &:: "has_value" <>$ capt_arg_payload + $+...$--> FollyOptional.has_value + ; -"folly" &:: "Optional" &:: "reset" <>$ capt_arg_payload + $+...$--> FollyOptional.assign_none ~desc:"folly::Optional::reset()" + ; -"folly" &:: "Optional" &:: "value" <>$ capt_arg_payload $+...$--> FollyOptional.value ; -"std" &:: "basic_string" &:: "data" <>$ capt_arg_payload $--> StdBasicString.data ; -"std" &:: "basic_string" &:: "~basic_string" <>$ capt_arg_payload $--> StdBasicString.destructor diff --git a/infer/tests/codetoanalyze/cpp/pulse/issues.exp b/infer/tests/codetoanalyze/cpp/pulse/issues.exp index eb2785cbb..8f5c7bfc3 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/issues.exp +++ b/infer/tests/codetoanalyze/cpp/pulse/issues.exp @@ -30,6 +30,11 @@ codetoanalyze/cpp/pulse/nullptr.cpp, deref_nullptr_bad, 2, NULLPTR_DEREFERENCE, codetoanalyze/cpp/pulse/nullptr.cpp, no_check_return_bad, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `may_return_nullptr` here,assigned,is the null pointer,use-after-lifetime part of the trace starts here,passed as argument to `may_return_nullptr`,return from call to `may_return_nullptr`,assigned,invalid access occurs here] codetoanalyze/cpp/pulse/nullptr.cpp, std_false_type_deref_bad, 3, 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/nullptr.cpp, std_true_type_deref_bad, 3, 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/optional.cpp, assign2_bad, 4, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `folly::Optional::operator=` here,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),is folly::None,use-after-lifetime part of the trace starts here,passed as argument to `folly::Optional::operator=`,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::operator=`,passed as argument to `folly::Optional::value()` (modelled),return from call to `folly::Optional::value()` (modelled),invalid access occurs here] +codetoanalyze/cpp/pulse/optional.cpp, assign_bad, 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 folly::None,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::operator=`,parameter `other` of folly::Optional::operator=,passed as argument to `folly::Optional::assign()` (modelled),return from call to `folly::Optional::assign()` (modelled),return from call to `folly::Optional::operator=`,passed as argument to `folly::Optional::value()` (modelled),return from call to `folly::Optional::value()` (modelled),invalid access occurs here] +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 folly::None,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(arg)` (modelled),return from call to `folly::Optional::Optional(arg)` (modelled),passed as argument to `folly::Optional::value()` (modelled),return from call to `folly::Optional::value()` (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 folly::None,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::value()` (modelled),return from call to `folly::Optional::value()` (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 folly::None,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->`,return from call to `folly::Optional::operator->`,invalid access occurs here] codetoanalyze/cpp/pulse/reference_wrapper.cpp, reference_wrapper_heap_bad, 2, USE_AFTER_DELETE, no_bucket, ERROR, [invalidation part of the trace starts here,variable `rw` declared here,when calling `getwrapperHeap` here,variable `a` declared here,passed as argument to `WrapsB::WrapsB`,assigned,return from call to `WrapsB::WrapsB`,when calling `WrapsB::~WrapsB` here,parameter `this` of WrapsB::~WrapsB,when calling `WrapsB::__infer_inner_destructor_~WrapsB` here,parameter `this` of WrapsB::__infer_inner_destructor_~WrapsB,was invalidated by `delete`,use-after-lifetime part of the trace starts here,variable `rw` declared here,passed as argument to `getwrapperHeap`,variable `a` declared here,passed as argument to `WrapsB::WrapsB`,assigned,return from call to `WrapsB::WrapsB`,passed as argument to `ReferenceWrapperHeap::ReferenceWrapperHeap`,parameter `a` of ReferenceWrapperHeap::ReferenceWrapperHeap,passed as argument to `WrapsB::getb`,return from call to `WrapsB::getb`,assigned,return from call to `ReferenceWrapperHeap::ReferenceWrapperHeap`,return from call to `getwrapperHeap`,invalid access occurs here] codetoanalyze/cpp/pulse/reference_wrapper.cpp, reference_wrapper_stack_bad, 2, USE_AFTER_LIFETIME, no_bucket, ERROR, [invalidation part of the trace starts here,variable `rw` declared here,when calling `getwrapperStack` here,variable `b` declared here,is the address of a stack variable `b` whose lifetime has ended,use-after-lifetime part of the trace starts here,variable `rw` declared here,passed as argument to `getwrapperStack`,variable `b` declared here,passed as argument to `ReferenceWrapperStack::ReferenceWrapperStack`,parameter `bref` of ReferenceWrapperStack::ReferenceWrapperStack,assigned,return from call to `ReferenceWrapperStack::ReferenceWrapperStack`,return from call to `getwrapperStack`,invalid access occurs here] codetoanalyze/cpp/pulse/returns.cpp, returns::return_literal_stack_reference_bad, 0, STACK_VARIABLE_ADDRESS_ESCAPE, no_bucket, ERROR, [variable `C++ temporary` declared here,returned here] diff --git a/infer/tests/codetoanalyze/cpp/pulse/optional.cpp b/infer/tests/codetoanalyze/cpp/pulse/optional.cpp new file mode 100644 index 000000000..b3de9e68c --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/pulse/optional.cpp @@ -0,0 +1,130 @@ +/* + * 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 + +namespace folly { + +template +class Optional; + +struct None { + enum class _secret { _token }; + explicit constexpr None(_secret) {} +}; +constexpr None none{None::_secret::_token}; + +template +class Optional { + public: + typedef Value value_type; + + constexpr Optional(); + + Optional(const Optional& src); + + constexpr Optional(const None&) noexcept; + + constexpr Optional(const Value& newValue); + + void assign(const None&); + + void assign(const Optional& src); + + Optional& operator=(None) noexcept { + reset(); + return *this; + } + + Optional& operator=(const Optional& other) { + assign(other); + return *this; + } + + template + Value& emplace(Args&&... args); + + void reset() noexcept; + + constexpr Value& value() &; + + constexpr bool has_value() const noexcept; + + constexpr explicit operator bool() const noexcept { return has_value(); } + + constexpr const Value* operator->() const { return &value(); } + + constexpr Value* operator->() { return &value(); } +}; +} // namespace folly + +int not_none_ok() { + folly::Optional foo{5}; + return foo.value(); +} + +int none_check_ok() { + folly::Optional foo{folly::none}; + if (foo) { + return foo.value(); + } + return -1; +} + +int none_no_check_bad() { + folly::Optional foo{folly::none}; + return foo.value(); +} + +int none_copy_ok() { + folly::Optional foo{5}; + folly::Optional bar{foo}; + return bar.value(); +} + +int none_copy_bad() { + folly::Optional foo{folly::none}; + folly::Optional bar{foo}; + return bar.value(); +} + +int assign_ok() { + folly::Optional foo{5}; + folly::Optional bar{foo}; + foo = folly::none; + return bar.value(); +} + +int assign_bad() { + folly::Optional foo{folly::none}; + folly::Optional bar{5}; + int sum = bar.value(); + bar = foo; + sum += bar.value(); + return sum; +} + +int assign2_bad() { + folly::Optional foo{5}; + int sum = foo.value(); + foo = folly::none; + sum += foo.value(); + return sum; +} + +struct State { + std::vector vec; +}; + +void emplace(folly::Optional state) { + if (state) { + state.emplace(); + } + auto pos = state->vec.begin(); +} + +void operator_arrow_bad() { emplace(folly::none); }