Index: clang-tidy/cppcoreguidelines/OwningMemoryCheck.h =================================================================== --- clang-tidy/cppcoreguidelines/OwningMemoryCheck.h +++ clang-tidy/cppcoreguidelines/OwningMemoryCheck.h @@ -24,17 +24,34 @@ class OwningMemoryCheck : public ClangTidyCheck { public: OwningMemoryCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : ClangTidyCheck(Name, Context), + LegacyResources( + Options.get("LegacyResources", + "::malloc;::aligned_alloc;::realloc;::calloc;::fopen")), + LegacyResourceConsumers(Options.get("LegacyResourceConsumers", + "::free;::realloc;::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,::realloc,::fopen`. + const std::string LegacyResources; + /// List of old C-style functions that consume or release resources. + /// Defaults to `::free,::realloc,::fclose`. + const std::string LegacyResourceConsumers; }; } // namespace cppcoreguidelines Index: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp =================================================================== --- clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ 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, "LegacyResources", LegacyResources); + 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(LegacyResources); + 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, since 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); @@ -161,6 +213,19 @@ return false; } +bool OwningMemoryCheck::handleLegacyConsumers(const BoundNodes &Nodes) { + // Result of matching for legacy consumer-functions like `::free()`. + const auto *LegacyConsumer = Nodes.getNodeAs("legacy_consumer"); + + 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: docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst =================================================================== --- docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst +++ 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 as generated resources. .. code-block:: c++ @@ -69,6 +70,32 @@ 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:: LegacyResources + + Semicolon-separated list of fully qualified names of legacy functions that create + resources but cannot introduce ``gsl::owner<>``. + Defaults to ``::malloc;::calloc,::realloc,::fopen``. + +.. option:: LegacyResourceConsumers + + Semicolon-separated list of fully qualified names of legacy functions expection + resource owners as pointer arguments but cannot introduce ``gsl::owner<>``. + Defaults to ``::free,::realloc,::fclose``. + + Limitations ----------- Index: test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp =================================================================== --- /dev/null +++ test/clang-tidy/cppcoreguidelines-owning-memory-legacy-functions.cpp @@ -0,0 +1,154 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-owning-memory %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: cppcoreguidelines-owning-memory.LegacyResources, value: "::malloc;::realloc;::calloc;::fopen"}, \ +// RUN: {key: cppcoreguidelines-owning-memory.LegacyResourceConsumers, value: "::realloc;::free;::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 *realloc(void *Resource, size_t NewByteCount); +void free(void *Resource); +FILE *fopen(const char *filename, const char *mode); +void fclose(FILE *Resource); +} + +namespace std { +using ::fclose; +using ::FILE; +using ::fopen; +using ::free; +using ::malloc; +using ::realloc; +using ::size_t; +} // 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+"); // Bad + // 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+"); // Bad + // 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 *') + + 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 +} + +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<>' +}