Index: clang-tidy/objc/AvoidOSReadCheck.h =================================================================== --- /dev/null +++ clang-tidy/objc/AvoidOSReadCheck.h @@ -0,0 +1,35 @@ +//===--- AvoidOSReadCheck.h - clang-tidy ------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_AVOIDOSREADCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_AVOIDOSREADCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace objc { + +/// Finds usages of OSRead{Big|Little}Int{16|32|64} and suggests using memcpy +/// instead due to potential misaligned read problems with the API. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/objc-avoid-osread.html +class AvoidOSReadCheck : public ClangTidyCheck { +public: + AvoidOSReadCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace objc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_AVOIDOSREADCHECK_H Index: clang-tidy/objc/AvoidOSReadCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/objc/AvoidOSReadCheck.cpp @@ -0,0 +1,40 @@ +//===--- AvoidOSReadCheck.cpp - clang-tidy --------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "AvoidOSReadCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace objc { + +void AvoidOSReadCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr(callee((functionDecl(hasAnyName( + "OSReadBigInt", "OSReadLittleInt", + "OSReadBigInt16", "OSReadBigInt32", "OSReadBigInt64", + "OSReadLittleInt16", "OSReadLittleInt32", "OSReadLittleInt64", + "OSReadSwapInt16", "OSReadSwapInt32", "OSReadSwapInt64", + "_OSReadInt16", "_OSReadInt32", "_OSReadInt64"))))) + .bind("osreadcall"), + this); +} + +void AvoidOSReadCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedExpr = Result.Nodes.getNodeAs("osreadcall"); + diag(MatchedExpr->getBeginLoc(), + "use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of " + "OSRead* calls to avoid potential unaligned read issues"); +} + +} // namespace objc +} // namespace tidy +} // namespace clang Index: clang-tidy/objc/CMakeLists.txt =================================================================== --- clang-tidy/objc/CMakeLists.txt +++ clang-tidy/objc/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_library(clangTidyObjCModule AvoidNSErrorInitCheck.cpp + AvoidOSReadCheck.cpp AvoidSpinlockCheck.cpp ForbiddenSubclassingCheck.cpp ObjCTidyModule.cpp Index: clang-tidy/objc/ObjCTidyModule.cpp =================================================================== --- clang-tidy/objc/ObjCTidyModule.cpp +++ clang-tidy/objc/ObjCTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "AvoidNSErrorInitCheck.h" +#include "AvoidOSReadCheck.h" #include "AvoidSpinlockCheck.h" #include "ForbiddenSubclassingCheck.h" #include "PropertyDeclarationCheck.h" @@ -26,6 +27,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "objc-avoid-nserror-init"); + CheckFactories.registerCheck( + "objc-avoid-osread"); CheckFactories.registerCheck( "objc-avoid-spinlock"); CheckFactories.registerCheck( Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -97,6 +97,12 @@ Now also checks if any calls to ``pthread_*`` functions expect negative return values. +- New :doc:`objc-avoid-osread + ` check. + + Finds uses of ``OSRead{Big|Little}Int{16|32|64}`` calls on Apple platforms that may mask + unexpected behavior due to unaligned reads. + Improvements to include-fixer ----------------------------- Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -323,6 +323,7 @@ mpi-buffer-deref mpi-type-mismatch objc-avoid-nserror-init + objc-avoid-osread objc-avoid-spinlock objc-forbidden-subclassing objc-property-declaration Index: docs/clang-tidy/checks/objc-avoid-osread.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/objc-avoid-osread.rst @@ -0,0 +1,40 @@ +.. title:: clang-tidy - objc-avoid-osread + +objc-avoid-osread +================= + +Finds usages of ``OSRead{Big|Little}Int{16|32|64}`` and associated functions which +should be avoided due to potential unaligned read problems. + +This check will detect following function invocations: + +- ``OSReadBigInt`` +- ``OSReadLittleInt`` +- ``OSReadBigInt16`` +- ``OSReadBigInt32`` +- ``OSReadBigInt64`` +- ``OSReadLittleInt16`` +- ``OSReadLittleInt32`` +- ``OSReadLittleInt64`` +- ``OSReadSwapInt16`` +- ``OSReadSwapInt3`` +- ``OSReadSwapInt64`` +- ``_OSReadInt16`` +- ``_OSReadInt32`` +- ``_OSReadInt64`` + +The ``OSRead{Big|Little}Int{16|32|64}`` functions from ``libkern/OSByteOrder.h`` have +undocumented dependencies on aligned reads due to type punning by casting through a +pointer: + +.. code:: c + + OS_INLINE uint64_t _OSReadInt64(const volatile void *base, uintptr_t byteOffset) + { + return *(volatile uint64_t *)((uintptr_t)base + byteOffset); + } + +This is undefined behavior for unaligned accesses and can cause crashes on architectures +that don't handle unaligned accesses well like ARMv7. + +https://gist.github.com/dmaclach/b10b0a71ae614d304c067cb9bd264336 shows the problem. Index: test/clang-tidy/objc-avoid-osread.m =================================================================== --- /dev/null +++ test/clang-tidy/objc-avoid-osread.m @@ -0,0 +1,35 @@ +// RUN: %check_clang_tidy %s objc-avoid-osread %t + +void f() { + const char *buff = ""; + OSReadBigInt(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadBigInt(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadLittleInt(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadBigInt16(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadBigInt32(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadBigInt64(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadLittleInt16(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadLittleInt32(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadLittleInt64(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadSwapInt16(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadSwapInt32(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + OSReadSwapInt64(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + _OSReadInt16(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + _OSReadInt32(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] + _OSReadInt64(buff, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use memcpy and OSSwap{Big|Little}ToHostInt{16|32|64} instead of OSRead* calls to avoid potential unaligned read issues [objc-avoid-osread] +}