Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.h +++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.h @@ -24,17 +24,36 @@ class OwningMemoryCheck : public ClangTidyCheck { public: OwningMemoryCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : ClangTidyCheck(Name, Context), + LegacyResourceProducers(Options.get( + "LegacyResourceProducers", "::malloc;::aligned_alloc;::realloc;" + "::calloc;::fopen;::freopen;::tmpfile")), + LegacyResourceConsumers(Options.get( + "LegacyResourceConsumers", "::free;::realloc;::freopen;::fclose")) { + } + + /// Make configuration of checker discoverable. + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: bool handleDeletion(const ast_matchers::BoundNodes &Nodes); + bool handleLegacyConsumers(const ast_matchers::BoundNodes &Nodes); bool handleExpectedOwner(const ast_matchers::BoundNodes &Nodes); bool handleAssignmentAndInit(const ast_matchers::BoundNodes &Nodes); bool handleAssignmentFromNewOwner(const ast_matchers::BoundNodes &Nodes); bool handleReturnValues(const ast_matchers::BoundNodes &Nodes); bool handleOwnerMembers(const ast_matchers::BoundNodes &Nodes); + + /// List of old C-style functions that create resources. + /// Defaults to + /// `::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile`. + const std::string LegacyResourceProducers; + /// List of old C-style functions that consume or release resources. + /// Defaults to `::free;::realloc;::freopen;::fclose`. + const std::string LegacyResourceConsumers; }; } // namespace cppcoreguidelines Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp @@ -22,6 +22,21 @@ namespace tidy { namespace cppcoreguidelines { +// FIXME: Copied from 'NoMallocCheck.cpp'. Has to be refactored into 'util' or +// something like that. +namespace { +Matcher hasAnyListedName(const std::string &FunctionNames) { + const std::vector NameList = + utils::options::parseStringList(FunctionNames); + return hasAnyName(std::vector(NameList.begin(), NameList.end())); +} +} // namespace + +void OwningMemoryCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "LegacyResourceProducers", LegacyResourceProducers); + Options.store(Opts, "LegacyResourceConsumers", LegacyResourceConsumers); +} + /// Match common cases, where the owner semantic is relevant, like function /// calls, delete expressions and others. void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { @@ -30,10 +45,31 @@ const auto OwnerDecl = typeAliasTemplateDecl(hasName("::gsl::owner")); const auto IsOwnerType = hasType(OwnerDecl); + + const auto LegacyCreatorFunctions = hasAnyListedName(LegacyResourceProducers); + const auto LegacyConsumerFunctions = + hasAnyListedName(LegacyResourceConsumers); + + // Legacy functions that are use for resource management but cannot be + // updated to use `gsl::owner<>`, like standard C memory management. + const auto CreatesLegacyOwner = + callExpr(callee(functionDecl(LegacyCreatorFunctions))); + // C-style functions like `::malloc()` sometimes create owners as void* + // which is expected to be cast to the correct type in C++. This case + // must be catched explicitly. + const auto LegacyOwnerCast = + castExpr(hasSourceExpression(CreatesLegacyOwner)); + // Functions that do manual resource management but cannot be updated to use + // owner. Best example is `::free()`. + const auto LegacyOwnerConsumers = functionDecl(LegacyConsumerFunctions); + const auto CreatesOwner = - anyOf(cxxNewExpr(), callExpr(callee(functionDecl( - returns(qualType(hasDeclaration(OwnerDecl))))))); - const auto ConsideredOwner = anyOf(IsOwnerType, CreatesOwner); + anyOf(cxxNewExpr(), + callExpr(callee( + functionDecl(returns(qualType(hasDeclaration(OwnerDecl)))))), + CreatesLegacyOwner, LegacyOwnerCast); + + const auto ConsideredOwner = eachOf(IsOwnerType, CreatesOwner); // Find delete expressions that delete non-owners. Finder->addMatcher( @@ -43,6 +79,21 @@ .bind("delete_expr"), this); + // Ignoring the implicit casts is vital because the legacy owners do not work + // with the 'owner<>' annotation and therefore always implicitly cast to the + // legacy type (even 'void *'). + // + // Furthermore, legacy owner functions are assumed to use raw pointers for + // resources. This check assumes that all pointer arguments of a legacy + // functions shall be 'gsl::owner<>'. + Finder->addMatcher( + callExpr( + allOf(callee(LegacyOwnerConsumers), + hasAnyArgument(allOf(unless(ignoringImpCasts(ConsideredOwner)), + hasType(pointerType()))))) + .bind("legacy_consumer"), + this); + // Matching assignment to owners, with the rhs not being an owner nor creating // one. Finder->addMatcher(binaryOperator(allOf(matchers::isAssignmentOperator(), @@ -133,6 +184,7 @@ bool CheckExecuted = false; CheckExecuted |= handleDeletion(Nodes); + CheckExecuted |= handleLegacyConsumers(Nodes); CheckExecuted |= handleExpectedOwner(Nodes); CheckExecuted |= handleAssignmentAndInit(Nodes); CheckExecuted |= handleAssignmentFromNewOwner(Nodes); @@ -168,6 +220,22 @@ return false; } +bool OwningMemoryCheck::handleLegacyConsumers(const BoundNodes &Nodes) { + // Result of matching for legacy consumer-functions like `::free()`. + const auto *LegacyConsumer = Nodes.getNodeAs("legacy_consumer"); + + // FIXME: `freopen` should be handled seperately because it takes the filename + // as a pointer, which should not be an owner. The argument that is an owner + // is known and the false positive coming from the filename can be avoided. + if (LegacyConsumer) { + diag(LegacyConsumer->getLocStart(), + "calling legacy resource function without passing a 'gsl::owner<>'") + << LegacyConsumer->getSourceRange(); + return true; + } + return false; +} + bool OwningMemoryCheck::handleExpectedOwner(const BoundNodes &Nodes) { // Result of function call matchers. const auto *ExpectedOwner = Nodes.getNodeAs("expected_owner_argument"); Index: clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst @@ -20,7 +20,8 @@ All checks are purely type based and not (yet) flow sensitive. The following examples will demonstrate the correct and incorrect initializations -of owners, assignment is handled the same way. +of owners, assignment is handled the same way. Note that both ``new`` and +``malloc()``-like resource functions are considered to produce resources. .. code-block:: c++ @@ -69,6 +70,33 @@ expects_owner(Owner); // Good expects_owner(new int(42)); // Good as well, recognized created resource + // Port legacy code for better resource-safety + gsl::owner File = fopen("my_file.txt", "rw+"); + FILE* BadFile = fopen("another_file.txt", "w"); // Bad, warned + + // ... use the file + + fclose(File); // Ok, File is annotated as 'owner<>' + fclose(BadFile); // BadFile is not an 'owner<>', will be warned + + +Options +------- + +.. option:: LegacyResourceProducers + + Semicolon-separated list of fully qualified names of legacy functions that create + resources but cannot introduce ``gsl::owner<>``. + Defaults to ``::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile``. + + +.. option:: LegacyResourceConsumers + + Semicolon-separated list of fully qualified names of legacy functions expecting + resource owners as pointer arguments but cannot introduce ``gsl::owner<>``. + Defaults to ``::free;::realloc;::freopen;::fclose``. + + Limitations ----------- @@ -82,7 +110,8 @@ The ``gsl::owner`` is declared as a templated type alias. In template functions and classes, like in the example below, the information of the type aliases gets lost. Therefore using ``gsl::owner`` in a heavy templated -code base might lead to false positives. +code base might lead to false positives. +This limitation results in ``std::vector>`` not being diagnosed correctly. .. code-block:: c++ Index: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-containers.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-containers.cpp +++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-containers.cpp @@ -0,0 +1,61 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-owning-memory %t + +namespace gsl { +template +using owner = T; +} + +namespace std { + +// Not actually a vector, but more a dynamic, fixed size array. Just to demonstrate +// functionality or the lack of the same. +template +class vector { +public: + vector(unsigned long size, T val) : data{new T[size]}, size{size} { + for (unsigned long i = 0ul; i < size; ++i) { + data[i] = val; + } + } + + T *begin() { return data; } + T *end() { return &data[size]; } + T &operator[](unsigned long index) { return data[index]; } + +private: + T *data; + unsigned long size; +}; + +} // namespace std + +// All of the following codesnippets should be valid with appropriate 'owner<>' anaylsis, +// but currently the type information of 'gsl::owner<>' gets lost in typededuction. +int main() { + std::vector> OwnerStdVector(100, nullptr); + + // Rangebased looping in resource vector. + for (auto *Element : OwnerStdVector) { + Element = new int(42); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: assigning newly created 'gsl::owner<>' to non-owner 'int *' + } + for (auto *Element : OwnerStdVector) { + delete Element; + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead + // CHECK-MESSAGES: [[@LINE-3]]:8: note: variable declared here + } + + // Indexbased looping in resource vector. + for (int i = 0; i < 100; ++i) { + OwnerStdVector[i] = new int(42); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: assigning newly created 'gsl::owner<>' to non-owner 'int *' + } + for (int i = 0; i < 100; ++i) { + delete OwnerStdVector[i]; + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead + // A note gets emitted here pointing to the return value of the operator[] from the + // vector implementation. Maybe this is considered misleading. + } + + return 0; +} Index: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp +++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp @@ -0,0 +1,194 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-owning-memory %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: cppcoreguidelines-owning-memory.LegacyResourceProducers, value: "::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile"}, \ +// RUN: {key: cppcoreguidelines-owning-memory.LegacyResourceConsumers, value: "::free;::realloc;::freopen;::fclose"}]}' \ +// RUN: -- -std=c++11 -nostdlib -nostdinc++ + +namespace gsl { +template +using owner = T; +} // namespace gsl + +extern "C" { +using size_t = unsigned long; +using FILE = int; + +void *malloc(size_t ByteCount); +void *aligned_alloc(size_t Alignment, size_t Size); +void *calloc(size_t Count, size_t SizeSingle); +void *realloc(void *Resource, size_t NewByteCount); +void free(void *Resource); + +FILE *tmpfile(void); +FILE *fopen(const char *filename, const char *mode); +FILE *freopen(const char *filename, const char *mode, FILE *stream); +void fclose(FILE *Resource); +} + +namespace std { +using ::FILE; +using ::size_t; + +using ::fclose; +using ::fopen; +using ::freopen; +using ::tmpfile; + +using ::aligned_alloc; +using ::calloc; +using ::free; +using ::malloc; +using ::realloc; +} // namespace std + +void nonOwningCall(int *Resource, size_t Size) {} +void nonOwningCall(FILE *Resource) {} + +void consumesResource(gsl::owner Resource, size_t Size) {} +void consumesResource(gsl::owner Resource) {} + +void testNonCasted(void *Resource) {} + +void testNonCastedOwner(gsl::owner Resource) {} + +FILE *fileFactory1() { return ::fopen("new_file.txt", "w"); } +// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'FILE *' (aka 'int *') or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' +gsl::owner fileFactory2() { return std::fopen("new_file.txt", "w"); } // Ok + +int *arrayFactory1() { return (int *)std::malloc(100); } +// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'int *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' +gsl::owner arrayFactory2() { return (int *)std::malloc(100); } // Ok +void *dataFactory1() { return std::malloc(100); } +// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'void *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' +gsl::owner dataFactory2() { return std::malloc(100); } // Ok + +void test_resource_creators() { + const unsigned int ByteCount = 25 * sizeof(int); + int Bad = 42; + + int *IntArray1 = (int *)std::malloc(ByteCount); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>' + int *IntArray2 = static_cast(std::malloc(ByteCount)); // Bad + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>' + void *IntArray3 = std::malloc(ByteCount); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>' + + int *IntArray4 = (int *)::malloc(ByteCount); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>' + int *IntArray5 = static_cast(::malloc(ByteCount)); // Bad + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>' + void *IntArray6 = ::malloc(ByteCount); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>' + + gsl::owner IntArray7 = (int *)malloc(ByteCount); // Ok + gsl::owner IntArray8 = malloc(ByteCount); // Ok + + gsl::owner IntArray9 = &Bad; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected initialization with value of type 'gsl::owner<>'; got 'int *' + + nonOwningCall((int *)malloc(ByteCount), 25); + // CHECK-MESSAGES: [[@LINE-1]]:24: warning: initializing non-owner argument of type 'int *' with a newly created 'gsl::owner<>' + nonOwningCall((int *)::malloc(ByteCount), 25); + // CHECK-MESSAGES: [[@LINE-1]]:24: warning: initializing non-owner argument of type 'int *' with a newly created 'gsl::owner<>' + + consumesResource((int *)malloc(ByteCount), 25); // Ok + consumesResource((int *)::malloc(ByteCount), 25); // Ok + + testNonCasted(malloc(ByteCount)); + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>' + testNonCastedOwner(gsl::owner(malloc(ByteCount))); // Ok + testNonCastedOwner(malloc(ByteCount)); // Ok + + FILE *File1 = std::fopen("test_name.txt", "w+"); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + FILE *File2 = ::fopen("test_name.txt", "w+"); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + + gsl::owner File3 = ::fopen("test_name.txt", "w+"); // Ok + + FILE *File4; + File4 = ::fopen("test_name.txt", "w+"); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: assigning newly created 'gsl::owner<>' to non-owner 'FILE *' (aka 'int *') + + gsl::owner File5; + File5 = ::fopen("test_name.txt", "w+"); // Ok + File5 = File1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected assignment source to be of type 'gsl::owner<>'; got 'FILE *' (aka 'int *') + + gsl::owner File6 = File1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected initialization with value of type 'gsl::owner<>'; got 'FILE *' (aka 'int *') + + FILE *File7 = tmpfile(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + gsl::owner File8 = tmpfile(); // Ok + + nonOwningCall(::fopen("test_name.txt", "r")); + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + nonOwningCall(std::fopen("test_name.txt", "r")); + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + + consumesResource(::fopen("test_name.txt", "r")); // Ok + + int *HeapPointer3 = (int *)aligned_alloc(16ul, 4ul * 32ul); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>' + gsl::owner HeapPointer4 = static_cast(aligned_alloc(16ul, 4ul * 32ul)); // Ok + + void *HeapPointer5 = calloc(10ul, 4ul); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>' + gsl::owner HeapPointer6 = calloc(10ul, 4ul); // Ok +} + +void test_legacy_consumers() { + int StackInteger = 42; + + int *StackPointer = &StackInteger; + int *HeapPointer1 = (int *)malloc(100); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>' + gsl::owner HeapPointer2 = (int *)malloc(100); + + std::free(StackPointer); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>' + std::free(HeapPointer1); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>' + std::free(HeapPointer2); // Ok + // CHECK MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>' + + // FIXME: the check complains about initialization of 'void *' with new created owner. + // This happens, because the argument of `free` is not marked as 'owner<>' (and cannot be), + // and the check will not figure out could be meant as owner. + // This property will probably never be fixed, because it is probably a rather rare + // use-case and 'owner<>' should be wrapped in RAII classes anyway! + std::free(std::malloc(100)); // Ok but silly :) + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>' + + // Demonstrate, that multi-argument functions are diagnosed as well. + std::realloc(StackPointer, 200); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>' + std::realloc(HeapPointer1, 200); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>' + std::realloc(HeapPointer2, 200); // Ok + std::realloc(std::malloc(100), 200); // Ok but silly + // CHECK-MESSAGES: [[@LINE-1]]:16: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>' + + fclose(fileFactory1()); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>' + fclose(fileFactory2()); // Ok, same as FIXME with `free(malloc(100))` applies here + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + + gsl::owner File1 = fopen("testfile.txt", "r"); // Ok + FILE *File2 = freopen("testfile.txt", "w", File1); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + // CHECK-MESSAGES: [[@LINE-2]]:17: warning: calling legacy resource function without passing a 'gsl::owner<>' + // FIXME: The warning for not passing and owner<> is a false positive since both the filename and the + // mode are not supposed to be owners but still pointers. The check is to coarse for + // this function. Maybe `freopen` gets special treatment. + + gsl::owner File3 = freopen("testfile.txt", "w", File2); // Bad, File2 no owner + // CHECK-MESSAGES: [[@LINE-1]]:30: warning: calling legacy resource function without passing a 'gsl::owner<>' + + FILE *TmpFile = tmpfile(); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + FILE *File6 = freopen("testfile.txt", "w", TmpFile); // Bad, both return and argument + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>' + // CHECK-MESSAGES: [[@LINE-2]]:17: warning: calling legacy resource function without passing a 'gsl::owner<>' +}