[clang-tidy] Finds uses of OSRead* calls on macOS that may mask unexpected behavior due to unaligned reads

[dmaclach]

Summary

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}.

[Eugene.Zelenko]

clang-tidy/objc/AvoidOSReadCheck.cpp

22

Please add check if language is Objective-C.

docs/ReleaseNotes.rst

103

Please enclose OSRead* in double back-ticks.

docs/clang-tidy/checks/objc-avoid-osread.rst

7

Please synchronize with sentence in Release Notes.

30

Please separate with empty line.

[Eugene.Zelenko]

clang-tidy/objc/AvoidOSReadCheck.h

6

License was changed this year. Same in source file.

[dmaclach]

Fixed up review comments.

[dmaclach]

clang-tidy/objc/AvoidOSReadCheck.cpp

22

So this isn't specific to Objective C, and applies to all C based languages. This is more Darwin specific. I based this CL on the AvoidSpinlockCheck which is very similar in this respect. If we want to do clean up on this later we should probably do them together.

docs/clang-tidy/checks/objc-avoid-osread.rst

7

Done. I coped this version into the release notes.

[Eugene.Zelenko]

docs/clang-tidy/checks/objc-avoid-osread.rst

27

Please highlight OSRead* and libkern/OSByteOrder.h with double back-ticks. Please also make lines no more then 80 characters long.

[dmaclach]

Updated based on review

[dmaclach]

Any other comments on this?

[gribozavr]

What is the expected contract of the functions that this checker flags? Are they supposed to perform unaligned reads correctly, and we have just an implementation bug in these functions, or is it the caller's fault if they pass an unaligned address?

clang-tidy/objc/ObjCTidyModule.cpp

30

Maybe a better place for this checker is the new "darwin" module? It is being added in https://reviews.llvm.org/D67567.

test/clang-tidy/objc-avoid-osread.m

5

Please add declarations for these functions. It is strange that this test even works without them...

clang-tidy/objc/AvoidOSReadCheck.h

+//===--- 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

clang-tidy/objc/AvoidOSReadCheck.cpp

+//===--- 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<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

clang-tidy/objc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyObjCModule
   AvoidNSErrorInitCheck.cpp
+  AvoidOSReadCheck.cpp
   AvoidSpinlockCheck.cpp
   ForbiddenSubclassingCheck.cpp
   ObjCTidyModule.cpp
   PropertyDeclarationCheck.cpp
   SuperSelfCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/objc/ObjCTidyModule.cpp

 //===--- ObjCTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidNSErrorInitCheck.h"
+#include "AvoidOSReadCheck.h"
 #include "AvoidSpinlockCheck.h"
 #include "ForbiddenSubclassingCheck.h"
 #include "PropertyDeclarationCheck.h"
 #include "SuperSelfCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   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>(
         "objc-forbidden-subclassing");
     CheckFactories.registerCheck<PropertyDeclarationCheck>(
         "objc-property-declaration");
     CheckFactories.registerCheck<SuperSelfCheck>(
         "objc-super-self");

docs/ReleaseNotes.rst

- New :doc:`llvm-prefer-register-over-unsigned
   them to use ``Register``
 
 - Improved :doc:`bugprone-posix-return
   <clang-tidy/checks/bugprone-posix-return>` check.
 
   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{Big|Little}Int{16|32|64}`` calls on Apple platforms that may mask 
+  unexpected behavior due to unaligned reads.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-use-override
    modernize-use-trailing-return-type
    modernize-use-transparent-functors
    modernize-use-uncaught-exceptions
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    objc-avoid-nserror-init
+   objc-avoid-osread
    objc-avoid-spinlock
    objc-forbidden-subclassing
    objc-property-declaration
    objc-super-self
    openmp-exception-escape
    openmp-use-default-none
    performance-faster-string-find
    performance-for-range-copy

docs/clang-tidy/checks/objc-avoid-osread.rst

+.. 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.

test/clang-tidy/objc-avoid-osread.m

+// 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]
+}