diff --git a/Changelog b/Changelog --- a/Changelog +++ b/Changelog @@ -124,4 +124,5 @@ * v1.7 (, 2020) - New Checks: - overloaded signal + - invalid JNI signatures - qstring-arg warns when using QLatin1String::arg(int), as it casts to QChar diff --git a/CheckSources.cmake b/CheckSources.cmake --- a/CheckSources.cmake +++ b/CheckSources.cmake @@ -19,6 +19,7 @@ ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/thread-with-slots.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/tr-non-literal.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/unneeded-cast.cpp + ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/jnisignatures.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level0/connect-by-name.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level0/connect-non-signal.cpp ${CMAKE_CURRENT_LIST_DIR}/src/checks/level0/connect-not-normalized.cpp diff --git a/README.md b/README.md --- a/README.md +++ b/README.md @@ -234,6 +234,7 @@ - [thread-with-slots](docs/checks/README-thread-with-slots.md) - [tr-non-literal](docs/checks/README-tr-non-literal.md) - [unneeded-cast](docs/checks/README-unneeded-cast.md) + - [jni-sigantures](docs/checks/README-jni-signatures.md) - Checks from Level 0: - [connect-by-name](docs/checks/README-connect-by-name.md) diff --git a/checks.json b/checks.json --- a/checks.json +++ b/checks.json @@ -616,6 +616,12 @@ "level" : -1, "categories" : ["containers"], "visits_stmts" : true + }, + { + "name" : "jni-signatures", + "level" : -1, + "categories" : ["bug"], + "visits_stmts" : true } ] } diff --git a/clazy.cmake b/clazy.cmake --- a/clazy.cmake +++ b/clazy.cmake @@ -107,6 +107,18 @@ ExtraClangOptions="-Xclang -plugin-arg-clazy -Xclang visit-implicit-code" fi +if [[ $CLAZY_CHECKS == *"jni-signatures"* ]] +then + + if [[ -z $ANDROID_NDK ]] + then + echo "To test JNI signatures you need to set ANDROID_NDK to your Android NDK installation." + exit + fi + + ExtraClangOptions=$ExtraClangOptions" -idirafter"$ANDROID_NDK"/sysroot/usr/include" +fi + ClazyPluginLib=ClazyPlugin@CMAKE_SHARED_LIBRARY_SUFFIX@ if ( test -f "$libdir/$ClazyPluginLib" ) diff --git a/docs/checks/README-copyable-polymorphic.md b/docs/checks/README-copyable-polymorphic.md --- a/docs/checks/README-copyable-polymorphic.md +++ b/docs/checks/README-copyable-polymorphic.md @@ -21,7 +21,7 @@ (...) Derived d; -foo(d); +printNumber(d); ``` diff --git a/docs/checks/README-jni-signatures.md b/docs/checks/README-jni-signatures.md new file mode 100644 --- /dev/null +++ b/docs/checks/README-jni-signatures.md @@ -0,0 +1,11 @@ +# jni-signatures + +Tries to find invalid JNI signatures in QAndroidJniObject usage. Those will lead to runtime errors. + +#### Example +``` + QAndroidJniObject::callStaticMethod("toString", "()Ljava/lang/String"); // Should be '()Ljava/lang/String;' +``` + +#### Pitfalls +For this check to work you need to set the ANDROID_NDK environment variable to your Android NDK installation. diff --git a/docs/man/clazy.pod b/docs/man/clazy.pod --- a/docs/man/clazy.pod +++ b/docs/man/clazy.pod @@ -202,6 +202,9 @@ even if the -Werror compiler option is specified. This is useful if you want to use -Werror only for the regular gcc/clang warnings but not for clazy warnings. +B - comma-separated list of checks that will be promoted +to compiler errors. Note that this does not enable the checks specified here. + =head1 COPYRIGHT AND LICENSE Copyright (C) 2015-2017 Klaralvdalens Datakonsult AB, a KDAB Group company, diff --git a/src/Checks.h b/src/Checks.h --- a/src/Checks.h +++ b/src/Checks.h @@ -47,6 +47,7 @@ #include "checks/manuallevel/thread-with-slots.h" #include "checks/manuallevel/tr-non-literal.h" #include "checks/manuallevel/unneeded-cast.h" +#include "checks/manuallevel/jnisignatures.h" #include "checks/level0/connect-by-name.h" #include "checks/level0/connect-non-signal.h" #include "checks/level0/connect-not-normalized.h" @@ -143,6 +144,7 @@ registerCheck(check("thread-with-slots", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts | RegisteredCheck::Option_VisitsDecls)); registerCheck(check("tr-non-literal", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("unneeded-cast", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts)); + registerCheck(check("jni-signatures", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("connect-by-name", CheckLevel0, RegisteredCheck::Option_VisitsDecls)); registerCheck(check("connect-non-signal", CheckLevel0, RegisteredCheck::Option_Qt4Incompatible | RegisteredCheck::Option_VisitsStmts)); registerCheck(check("connect-not-normalized", CheckLevel0, RegisteredCheck::Option_VisitsStmts)); diff --git a/src/ClazyContext.h b/src/ClazyContext.h --- a/src/ClazyContext.h +++ b/src/ClazyContext.h @@ -156,6 +156,11 @@ return sm.isInFileID(loc, sm.getMainFileID()); } + bool treatAsError(const std::string &checkName) const + { + return clazy::contains(m_checksPromotedToErrors, checkName); + } + /** * We only enable it if a check needs it, for performance reasons */ @@ -174,6 +179,7 @@ PreProcessorVisitor *preprocessorVisitor = nullptr; SuppressionManager suppressionManager; const bool m_noWerror; + std::vector m_checksPromotedToErrors; bool m_visitsAllTypeDefs = false; clang::ParentMap *parentMap = nullptr; const ClazyOptions options; diff --git a/src/ClazyContext.cpp b/src/ClazyContext.cpp --- a/src/ClazyContext.cpp +++ b/src/ClazyContext.cpp @@ -20,6 +20,7 @@ */ #include "AccessSpecifierManager.h" +#include "checkmanager.h" #include "ClazyContext.h" #include "FixItExporter.h" #include "PreProcessorVisitor.h" @@ -44,6 +45,7 @@ , astContext(ci.getASTContext()) , sm(ci.getSourceManager()) , m_noWerror(getenv("CLAZY_NO_WERROR") != nullptr) // Allows user to make clazy ignore -Werror + , m_checksPromotedToErrors(CheckManager::instance()->checksAsErrors()) , options(opts) , extraOptions(clazy::splitString(getenv("CLAZY_EXTRA_OPTIONS"), ',')) , m_translationUnitPaths(translationUnitPaths) diff --git a/src/checkbase.cpp b/src/checkbase.cpp --- a/src/checkbase.cpp +++ b/src/checkbase.cpp @@ -251,8 +251,9 @@ { FullSourceLoc full(loc, sm()); auto &engine = m_context->ci.getDiagnostics(); - auto severity = (engine.getWarningsAsErrors() && !m_context->userDisabledWError()) ? DiagnosticIDs::Error - : DiagnosticIDs::Warning; + auto severity = (m_context->treatAsError(m_name) || (engine.getWarningsAsErrors() && !m_context->userDisabledWError())) + ? DiagnosticIDs::Error + : DiagnosticIDs::Warning; unsigned id = engine.getDiagnosticIDs()->getCustomDiagID(severity, error.c_str()); DiagnosticBuilder B = engine.Report(full, id); for (const FixItHint& fixit : fixits) { diff --git a/src/checkmanager.h b/src/checkmanager.h --- a/src/checkmanager.h +++ b/src/checkmanager.h @@ -93,6 +93,7 @@ static std::mutex &lock() { return m_lock; } RegisteredCheck::List availableChecks(CheckLevel maxLevel) const; RegisteredCheck::List requestedChecksThroughEnv(std::vector &userDisabledChecks) const; + std::vector checksAsErrors() const; RegisteredCheck::List::const_iterator checkForName(const RegisteredCheck::List &checks, const std::string &name) const; RegisteredCheck::List checksForCommaSeparatedString(const std::string &str) const; diff --git a/src/checkmanager.cpp b/src/checkmanager.cpp --- a/src/checkmanager.cpp +++ b/src/checkmanager.cpp @@ -317,3 +317,28 @@ return result; } + +vector CheckManager::checksAsErrors() const +{ + auto checksAsErrosEnv = getenv("CLAZY_CHECKS_AS_ERRORS"); + + if (checksAsErrosEnv) { + auto checkNames = clazy::splitString(checksAsErrosEnv, ','); + vector result; + + // Check whether all supplied check names are valid + for (const string &name : checkNames) { + auto it = clazy::find_if(m_registeredChecks, [&name](const RegisteredCheck &check) + { + return check.name == name; + }); + if (it == m_registeredChecks.end()) + llvm::errs() << "Invalid check: " << name << '\n'; + else + result.emplace_back(name); + } + return result; + } + else + return {}; +} diff --git a/src/checks/level0/overloaded-signal.cpp b/src/checks/level0/overloaded-signal.cpp --- a/src/checks/level0/overloaded-signal.cpp +++ b/src/checks/level0/overloaded-signal.cpp @@ -66,7 +66,7 @@ emitWarning(decl, "signal " + methodName.str() + " is overloaded"); continue; // No point in spitting more warnings for the same signal } else { - emitWarning(decl, "signal " + methodName.str() + " is overloaded (with " + p->getBeginLoc().printToString(sm()) + ")"); + emitWarning(decl, "signal " + methodName.str() + " is overloaded (with " + clazy::getLocStart(p).printToString(sm()) + ")"); } } } diff --git a/src/checks/level2/qstring-allocations.h b/src/checks/level2/qstring-allocations.h --- a/src/checks/level2/qstring-allocations.h +++ b/src/checks/level2/qstring-allocations.h @@ -68,6 +68,7 @@ private: void VisitCtor(clang::Stmt *); + void VisitCtor(clang::CXXConstructExpr *); void VisitOperatorCall(clang::Stmt *); void VisitFromLatin1OrUtf8(clang::Stmt *); void VisitAssignOperatorQLatin1String(clang::Stmt *); diff --git a/src/checks/level2/qstring-allocations.cpp b/src/checks/level2/qstring-allocations.cpp --- a/src/checks/level2/qstring-allocations.cpp +++ b/src/checks/level2/qstring-allocations.cpp @@ -176,14 +176,39 @@ void QStringAllocations::VisitCtor(Stmt *stm) { auto ctorExpr = dyn_cast(stm); + if (!ctorExpr) + return; + if (!Utils::containsStringLiteral(ctorExpr, /**allowEmpty=*/ true)) return; CXXConstructorDecl *ctorDecl = ctorExpr->getConstructor(); +#if LLVM_VERSION_MAJOR >= 10 + // With llvm 10, for some reason, the child CXXConstructExpr of QStringList foo = {"foo}; aren't visited :(. + // Do it manually. + if (clazy::isOfClass(ctorDecl, "QStringList")) { + auto p = clazy::getFirstChildOfType2(ctorExpr); + while (p) { + if (clazy::isOfClass(p, "QString")) { + VisitCtor(p); + } + p = clazy::getFirstChildOfType2(p); + } + } else { + VisitCtor(ctorExpr); + } +#else + VisitCtor(ctorExpr); +#endif +} + +void QStringAllocations::VisitCtor(CXXConstructExpr *ctorExpr) +{ + CXXConstructorDecl *ctorDecl = ctorExpr->getConstructor(); if (!clazy::isOfClass(ctorDecl, "QString")) return; - if (Utils::insideCTORCall(m_context->parentMap, stm, { "QRegExp", "QIcon" })) { + if (Utils::insideCTORCall(m_context->parentMap, ctorExpr, { "QRegExp", "QIcon" })) { // https://blogs.kde.org/2015/11/05/qregexp-qstringliteral-crash-exit return; } @@ -193,7 +218,7 @@ if (initializerList != nullptr) return; // Nothing to do here, MSVC doesn't like it - StringLiteral *lt = stringLiteralForCall(stm); + StringLiteral *lt = stringLiteralForCall(ctorExpr); if (lt && lt->getNumConcatenated() > 1) { return; // Nothing to do here, MSVC doesn't like it } @@ -214,7 +239,7 @@ if (isQLatin1String) { ConditionalOperator *ternary = nullptr; - Latin1Expr qlatin1expr = qlatin1CtorExpr(stm, ternary); + Latin1Expr qlatin1expr = qlatin1CtorExpr(ctorExpr, ternary); if (!qlatin1expr.isValid()) { return; } @@ -233,7 +258,7 @@ if (!clazy::getLocStart(qlatin1Ctor).isMacroID()) { if (!ternary) { fixits = fixItReplaceWordWithWord(qlatin1Ctor, "QStringLiteral", "QLatin1String"); - bool shouldRemoveQString = clazy::getLocStart(qlatin1Ctor).getRawEncoding() != clazy::getLocStart(stm).getRawEncoding() && dyn_cast_or_null(clazy::parent(m_context->parentMap, ctorExpr)); + bool shouldRemoveQString = clazy::getLocStart(qlatin1Ctor).getRawEncoding() != clazy::getLocStart(ctorExpr).getRawEncoding() && dyn_cast_or_null(clazy::parent(m_context->parentMap, ctorExpr)); if (shouldRemoveQString) { // This is the case of QString(QLatin1String("foo")), which we just fixed to be QString(QStringLiteral("foo)), so now remove QString auto removalFixits = clazy::fixItRemoveToken(&m_astContext, ctorExpr, true); @@ -251,7 +276,7 @@ } } - maybeEmitWarning(clazy::getLocStart(stm), msg, fixits); + maybeEmitWarning(clazy::getLocStart(ctorExpr), msg, fixits); } else { vector fixits; if (clazy::hasChildren(ctorExpr)) { @@ -293,7 +318,7 @@ } } - maybeEmitWarning(clazy::getLocStart(stm), msg, fixits); + maybeEmitWarning(clazy::getLocStart(ctorExpr), msg, fixits); } } diff --git a/src/checks/manuallevel/jnisignatures.h b/src/checks/manuallevel/jnisignatures.h new file mode 100644 --- /dev/null +++ b/src/checks/manuallevel/jnisignatures.h @@ -0,0 +1,52 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2020 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com + Author: Nicolas Fella + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Library General Public + License as published by the Free Software Foundation; either + version 2 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Library General Public License for more details. + + You should have received a copy of the GNU Library General Public License + along with this library; see the file COPYING.LIB. If not, write to + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + Boston, MA 02110-1301, USA. +*/ + +#ifndef JNISIGNATURES_H +#define JNISIGNATURES_H + +#include "checkbase.h" + +#include + +class ClazyContext; +namespace clang { +class Stmt; +class CXXMemberCallExpr; +class FunctionDecl; +} // namespace clang + +class JniSignatures + : public CheckBase +{ +public: + JniSignatures(const std::string &name, ClazyContext *context); + void VisitStmt(clang::Stmt *) override; +private: + template + void checkArgAt(T *call, unsigned int index); + void checkConstructorCall(clang::Stmt *stm); + void checkFunctionCall(clang::Stmt *stm); + bool functionShouldBeChecked(clang::FunctionDecl *funDecl); +}; + +#endif + diff --git a/src/checks/manuallevel/jnisignatures.cpp b/src/checks/manuallevel/jnisignatures.cpp new file mode 100644 --- /dev/null +++ b/src/checks/manuallevel/jnisignatures.cpp @@ -0,0 +1,138 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2020 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com + Author: Nicolas Fella + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Library General Public + License as published by the Free Software Foundation; either + version 2 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Library General Public License for more details. + + You should have received a copy of the GNU Library General Public License + along with this library; see the file COPYING.LIB. If not, write to + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + Boston, MA 02110-1301, USA. +*/ + +#include "jnisignatures.h" +#include "MacroUtils.h" +#include "FunctionUtils.h" +#include "StringUtils.h" +#include "SourceCompatibilityHelpers.h" +#include "clazy_stl.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +class ClazyContext; + +using namespace clang; +using namespace std; + +JniSignatures::JniSignatures(const std::string &name, ClazyContext *context) + : CheckBase(name, context, Option_CanIgnoreIncludes) +{ +} + +bool checkSignature(std::string signature) +{ + static regex rx("\\((\\[?([ZBCSIJFD]|L([a-zA-Z]+\\/)*[a-zA-Z]+;))*\\)\\[?([ZBCSIJFD]|L([a-zA-Z]+\\/)*[a-zA-Z]+;|V)"); + + smatch match; + return regex_match(signature, match, rx); +} + +template +void JniSignatures::checkArgAt(T *call, unsigned int index) +{ + if (call->getNumArgs() < index + 1) + return; + + StringLiteral *stringLiteral = clazy::getFirstChildOfType2(call->getArg(index)); + + if (!stringLiteral) + return; + + if (stringLiteral->getCharByteWidth() != 1) + return; + + const std::string signature = stringLiteral->getString().str(); + + const bool valid = checkSignature(signature); + + if (!valid) { + emitWarning(call, "Invalid method signature: '" + signature + "'"); + } +} + +bool JniSignatures::functionShouldBeChecked(FunctionDecl *funDecl) +{ + const std::string qualifiedName = funDecl->getQualifiedNameAsString(); + if (!clazy::startsWith(qualifiedName, "QAndroidJniObject::")) { + return false; + } + + const std::string name = clazy::name(funDecl); + return name == "callObjectMethod" + || name == "callMethod" + || name == "callStaticObjectMethod" + || name == "callStaticMethod"; +} + +void JniSignatures::checkFunctionCall(Stmt *stm) +{ + auto callExpr = dyn_cast(stm); + if (!callExpr) + return; + auto funDecl = callExpr->getDirectCallee(); + if (!funDecl) { + return; + } + + if (!functionShouldBeChecked(funDecl)) { + return; + } + + checkArgAt(callExpr, 1); +} + +void JniSignatures::checkConstructorCall(Stmt *stm) +{ + auto constructExpr = dyn_cast(stm); + if (!constructExpr) { + return; + } + auto funDecl = constructExpr->getConstructor(); + + const std::string qualifiedName = funDecl->getQualifiedNameAsString(); + if (qualifiedName != "QAndroidJniObject::QAndroidJniObject") { + return; + } + + checkArgAt(constructExpr, 1); +} + +void JniSignatures::VisitStmt(Stmt *stm) +{ + checkConstructorCall(stm); + checkFunctionCall(stm); +} diff --git a/tests/jni-signatures/config.json b/tests/jni-signatures/config.json new file mode 100644 --- /dev/null +++ b/tests/jni-signatures/config.json @@ -0,0 +1,8 @@ +{ + "tests" : [ + { + "filename" : "main.cpp" + } + ] +} + diff --git a/tests/jni-signatures/main.cpp b/tests/jni-signatures/main.cpp new file mode 100644 --- /dev/null +++ b/tests/jni-signatures/main.cpp @@ -0,0 +1,35 @@ + +// Crude approximation of #include so we don't need QtAndroidExtras installed +class QAndroidJniObject +{ + public: + QAndroidJniObject(const char *className, const char *signature); + + template + void callObjectMethod(const char *methodName, const char *signature); + + template + void callMethod(const char *methodName, const char *signature); + + template + static void callStaticMethod(const char *methodName, const char *signature); + + template + static void callStaticObjectMethod(const char *methodName, const char *signature); +}; + +int main() { + + QAndroidJniObject obj("java/lang/String", "("); + obj.callObjectMethod("someMethod", "()V"); + obj.callObjectMethod("someMethod", "(III)V"); + obj.callObjectMethod("someMethod", "(III"); + + obj.callMethod("someMethod", "()V"); + obj.callMethod("someMethod", "(III)V"); + obj.callMethod("someMethod", "()Ljava/lang/String"); + + QAndroidJniObject::callStaticMethod("someMethod", "()Ljava/lang/String"); + + QAndroidJniObject::callStaticObjectMethod("someOtherMethod", "(II)"); +} diff --git a/tests/jni-signatures/main.cpp.expected b/tests/jni-signatures/main.cpp.expected new file mode 100644 --- /dev/null +++ b/tests/jni-signatures/main.cpp.expected @@ -0,0 +1,5 @@ +jni-signatures/main.cpp:23:23: warning: Invalid method signature: '(' [-Wclazy-jni-signatures] +jni-signatures/main.cpp:26:5: warning: Invalid method signature: '(III' [-Wclazy-jni-signatures] +jni-signatures/main.cpp:30:5: warning: Invalid method signature: '()Ljava/lang/String' [-Wclazy-jni-signatures] +jni-signatures/main.cpp:32:5: warning: Invalid method signature: '()Ljava/lang/String' [-Wclazy-jni-signatures] +jni-signatures/main.cpp:34:5: warning: Invalid method signature: '(II)' [-Wclazy-jni-signatures]