dmaclach created this revision.
dmaclach added a reviewer: alexfh.
Herald added subscribers: cfe-commits, kristof.beyls, xazax.hun, mgorny.
Herald added a project: clang.
We ran into a problem in protocol buffers recently when compiling with Xcode 11
that caused crashes on 32 bit ARM architectures due to undefined behavior
caused by OSRead* calls in OSByteOrder.h when reading unaligned addresses.
These potential errors can easily be fixed by replacing the OSRead* call with a
memcpy and then a OSSwap{Big|Little}ToHostInt{16|32|64}.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D67865
Files:
clang-tidy/objc/AvoidOSReadCheck.cpp
clang-tidy/objc/AvoidOSReadCheck.h
clang-tidy/objc/CMakeLists.txt
clang-tidy/objc/ObjCTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/objc-avoid-osread.rst
test/clang-tidy/objc-avoid-osread.m
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]
+}
Index: docs/clang-tidy/checks/objc-avoid-osread.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/objc-avoid-osread.rst
@@ -0,0 +1,36 @@
+.. 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* 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: 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/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
+ <clang-tidy/checks/objc-avoid-osread>` check.
+
+ Finds uses of OSRead* calls on macOS that may mask unexpected behavior due to
+ unaligned reads.
+
Improvements to include-fixer
-----------------------------
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<AvoidNSErrorInitCheck>(
"objc-avoid-nserror-init");
+ CheckFactories.registerCheck<AvoidOSReadCheck>(
+ "objc-avoid-osread");
CheckFactories.registerCheck<AvoidSpinlockCheck>(
"objc-avoid-spinlock");
CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
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/AvoidOSReadCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/objc/AvoidOSReadCheck.h
@@ -0,0 +1,36 @@
+//===--- AvoidOSReadCheck.h - clang-tidy ------------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#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,41 @@
+//===--- AvoidOSReadCheck.cpp - clang-tidy --------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#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<CallExpr>("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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
