From 40c84077d9cd62800a62e90925bbc4833a78c422 Mon Sep 17 00:00:00 2001 From: Andrzej Kotulski Date: Thu, 2 Feb 2017 03:17:25 -0800 Subject: [PATCH] [C++] Fix model of std::vector Summary: Infer used to report null dereference when field was accessed later: ``` vector v; int& a = v[0]; // should be EMPTY_VECTOR_ACCESS here, but it wasn't reported int b = a; // was NULL_DEREFERENCE here ``` To avoid this problem, model all accesses to vector as dereference of its internal `beginPtr` field. Reviewed By: jberdine Differential Revision: D4481942 fbshipit-source-id: 2142894 --- infer/models/cpp/include/infer_model/vector.h | 22 +++++++- infer/src/backend/InferPrint.re | 3 +- .../tests/codetoanalyze/cpp/errors/issues.exp | 14 ++++- .../cpp/errors/vector/access_field_later.cpp | 56 +++++++++++++++++++ 4 files changed, 89 insertions(+), 6 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/errors/vector/access_field_later.cpp diff --git a/infer/models/cpp/include/infer_model/vector.h b/infer/models/cpp/include/infer_model/vector.h index 8b3df338b..960817636 100644 --- a/infer/models/cpp/include/infer_model/vector.h +++ b/infer/models/cpp/include/infer_model/vector.h @@ -19,6 +19,7 @@ #include #include +#include #ifdef INFER_USE_LIBCPP // libc++ vector header includes it, but it breaks @@ -53,6 +54,9 @@ struct vector_ref { template T* __infer_skip__get_nondet_val() {} +template +void __infer_deref_first_arg(T* ptr) INFER_MODEL_AS_DEREF_FIRST_ARG; + // WARNING: do not add any new fields to std::vector model. sizeof(std::vector) // = 24 !! template > @@ -82,7 +86,14 @@ class vector { value_type* endPtr = nullptr; value_type* __ignore; - value_type* get() const { return beginPtr; } + value_type* get() const { + // we have to resort to that hack so that infer can truly dereference + // beginPtr. + // Another way to model that would be 'auto tmp = *beginPtr' but that will + // trigger copy constructor that may not exist for value_type + __infer_deref_first_arg(beginPtr); + return beginPtr; + } void allocate(size_type size) { // assume that allocation will produce non-empty vector regardless of the @@ -98,11 +109,16 @@ class vector { template void allocate_iter(Iter begin, Iter end) { - if (begin != end) { + // very simplified implementation to avoid false positives + allocate(1); + // infer doesn't understand iterators well which leads to skip functions + // they in effect lead to false positives in situations when empty vector + // is impossible. Implemenatation should look like this: + /*if (begin != end) { allocate(1); } else { deallocate(); - } + }*/ } /* std::vector implementation */ diff --git a/infer/src/backend/InferPrint.re b/infer/src/backend/InferPrint.re index 3b4506e29..426ba4ffa 100644 --- a/infer/src/backend/InferPrint.re +++ b/infer/src/backend/InferPrint.re @@ -344,7 +344,8 @@ let should_report (issue_kind: Exceptions.err_kind) issue_type error_desc eclass field_not_null_checked, null_dereference, parameter_not_null_checked, - premature_nil_termination + premature_nil_termination, + empty_vector_access ]; List.mem equal::Localise.equal null_deref_issue_types issue_type }; diff --git a/infer/tests/codetoanalyze/cpp/errors/issues.exp b/infer/tests/codetoanalyze/cpp/errors/issues.exp index 95600a4e9..7277ca700 100644 --- a/infer/tests/codetoanalyze/cpp/errors/issues.exp +++ b/infer/tests/codetoanalyze/cpp/errors/issues.exp @@ -118,6 +118,16 @@ codetoanalyze/cpp/errors/subtyping/dynamic_cast.cpp, dynamic__cast::wrongPointer codetoanalyze/cpp/errors/subtyping/dynamic_cast.cpp, dynamic__cast::wrongReferenceCast, 3, CLASS_CAST_EXCEPTION, [start of procedure dynamic__cast::wrongReferenceCast(),start of procedure Base,return from a call to dynamic__cast::Base_Base] codetoanalyze/cpp/errors/subtyping/dynamic_cast.cpp, dynamic__cast::wrongReferenceCastNotAssigned, 3, CLASS_CAST_EXCEPTION, [start of procedure dynamic__cast::wrongReferenceCastNotAssigned(),start of procedure Base,return from a call to dynamic__cast::Base_Base] codetoanalyze/cpp/errors/subtyping/subtyping_check.cpp, B_setFG, 4, DIVIDE_BY_ZERO, [start of procedure setFG,start of procedure setF,return from a call to A_setF,Condition is true] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getIntPtr, 1, RETURN_VALUE_IGNORED, [start of procedure getIntPtr()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getIntPtr, 2, EMPTY_VECTOR_ACCESS, [start of procedure getIntPtr()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithCopy, 1, RETURN_VALUE_IGNORED, [start of procedure getWithCopy()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithCopy, 2, EMPTY_VECTOR_ACCESS, [start of procedure getWithCopy()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithCopyPtr, 1, RETURN_VALUE_IGNORED, [start of procedure getWithCopyPtr()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithCopyPtr, 2, EMPTY_VECTOR_ACCESS, [start of procedure getWithCopyPtr()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithoutCopy, 1, RETURN_VALUE_IGNORED, [start of procedure getWithoutCopy()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithoutCopy, 2, EMPTY_VECTOR_ACCESS, [start of procedure getWithoutCopy()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithoutCopyPtr, 1, RETURN_VALUE_IGNORED, [start of procedure getWithoutCopyPtr()] +codetoanalyze/cpp/errors/vector/access_field_later.cpp, getWithoutCopyPtr, 2, EMPTY_VECTOR_ACCESS, [start of procedure getWithoutCopyPtr()] codetoanalyze/cpp/errors/vector/empty_access.cpp, access_empty, 2, EMPTY_VECTOR_ACCESS, [start of procedure access_empty()] codetoanalyze/cpp/errors/vector/empty_access.cpp, assign_empty, 4, EMPTY_VECTOR_ACCESS, [start of procedure assign_empty()] codetoanalyze/cpp/errors/vector/empty_access.cpp, clear_empty, 3, EMPTY_VECTOR_ACCESS, [start of procedure clear_empty()] @@ -125,9 +135,9 @@ codetoanalyze/cpp/errors/vector/empty_access.cpp, copy_empty, 3, EMPTY_VECTOR_AC codetoanalyze/cpp/errors/vector/empty_access.cpp, empty_check_access_empty, 2, EMPTY_VECTOR_ACCESS, [start of procedure empty_check_access_empty(),Condition is true] codetoanalyze/cpp/errors/vector/empty_access.cpp, getter_empty, 0, EMPTY_VECTOR_ACCESS, [start of procedure getter_empty(),start of procedure get_vector(),return from a call to get_vector] codetoanalyze/cpp/errors/vector/empty_access.cpp, size_check0_empty, 2, EMPTY_VECTOR_ACCESS, [start of procedure size_check0_empty(),Condition is true] -codetoanalyze/cpp/errors/vector/empty_access.cpp, vector_as_param_by_value_empty, 2, EMPTY_VECTOR_ACCESS, [start of procedure vector_as_param_by_value_empty(),start of procedure vector_param_by_value_access()] +codetoanalyze/cpp/errors/vector/empty_access.cpp, vector_as_param_by_value_empty, 2, EMPTY_VECTOR_ACCESS, [start of procedure vector_as_param_by_value_empty(),start of procedure vector_param_by_value_access(),return from a call to vector_param_by_value_access] codetoanalyze/cpp/errors/vector/empty_access.cpp, vector_as_param_clear, 3, EMPTY_VECTOR_ACCESS, [start of procedure vector_as_param_clear(),start of procedure vector_param_clear(),return from a call to vector_param_clear] -codetoanalyze/cpp/errors/vector/empty_access.cpp, vector_as_param_empty, 2, EMPTY_VECTOR_ACCESS, [start of procedure vector_as_param_empty(),start of procedure vector_param_access()] +codetoanalyze/cpp/errors/vector/empty_access.cpp, vector_as_param_empty, 2, EMPTY_VECTOR_ACCESS, [start of procedure vector_as_param_empty(),start of procedure vector_param_access(),return from a call to vector_param_access] codetoanalyze/cpp/errors/vector/iterator_access.cpp, iterator_access::possible_npe, 4, NULL_DEREFERENCE, [start of procedure iterator_access::possible_npe(),Condition is true,Condition is true,Condition is true] codetoanalyze/cpp/shared/attributes/deprecated_hack.cpp, derefFirstArg2_null_deref, 2, NULL_DEREFERENCE, [start of procedure derefFirstArg2_null_deref()] codetoanalyze/cpp/shared/attributes/deprecated_hack.cpp, derefFirstArg3_null_deref, 2, NULL_DEREFERENCE, [start of procedure derefFirstArg3_null_deref(),start of procedure derefFirstArg3()] diff --git a/infer/tests/codetoanalyze/cpp/errors/vector/access_field_later.cpp b/infer/tests/codetoanalyze/cpp/errors/vector/access_field_later.cpp new file mode 100644 index 000000000..3cdccf56f --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/errors/vector/access_field_later.cpp @@ -0,0 +1,56 @@ +/* + * Copyright (c) 2017 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ +#include + +// has copy constructor +struct WithCopy { + int f1; + int f2; +}; + +// no copy constructor - important to catch wrong implementations of std::vector +// model +struct WithoutCopy { + WithoutCopy() = default; + WithoutCopy(WithoutCopy&& x) = default; + WithoutCopy(const WithoutCopy& x) = delete; + WithoutCopy& operator=(const WithoutCopy&) = delete; + int f1; + int f2; +}; + +int getIntPtr(int id, std::vector& v) { + int x = v.size(); + int** res = &v[id]; + return **res; +} + +int getWithCopy(int id, std::vector& v) { + int x = v.size(); + WithCopy* res = &v[id]; + return res->f1; +} + +int getWithCopyPtr(int id, std::vector& v) { + int x = v.size(); + WithCopy** res = &v[id]; + return (*res)->f1; +} + +int getWithoutCopy(int id, std::vector& v) { + int x = v.size(); + WithoutCopy* res = &v[id]; + return res->f1; +} + +int getWithoutCopyPtr(int id, std::vector& v) { + int x = v.size(); + WithoutCopy** res = &v[id]; + return (*res)->f1; +}