[clang-tidy] new check: bugprone-posix-return

Summary:
Checks if any calls to posix functions (except posix_openpt) expect negative return values.
These functions return either 0 on success or an errno on failure, which is positive only.

Reviewers: JonasToth, gribozavr, alexfh, hokein

Reviewed By: gribozavr

Subscribers: Eugene.Zelenko, lebedev.ri, llozano, george.burgess.iv, xazax.hun, srhines, mgorny, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D63623

Patch by Jian Cai.

llvm-svn: 365007

Author: gribozavr

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -31,6 +31,7 @@
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "ParentVirtualCallCheck.h"
+#include "PosixReturnCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StringConstructorCheck.h"
@@ -104,6 +105,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
+    CheckFactories.registerCheck<PosixReturnCheck>(
+        "bugprone-posix-return");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -23,6 +23,7 @@ add_clang_library(clangTidyBugproneModule
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   ParentVirtualCallCheck.cpp
+  PosixReturnCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StringConstructorCheck.cpp

clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp

@@ -0,0 +1,82 @@
+//===--- PosixReturnCheck.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 "PosixReturnCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+static StringRef getFunctionSpelling(const MatchFinder::MatchResult &Result,
+                                     const char *BindingStr) {
+  const CallExpr *MatchedCall = cast<CallExpr>(
+      (Result.Nodes.getNodeAs<BinaryOperator>(BindingStr))->getLHS());
+  const SourceManager &SM = *Result.SourceManager;
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(
+                                  MatchedCall->getCallee()->getSourceRange()),
+                              SM, Result.Context->getLangOpts());
+}
+
+void PosixReturnCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(binaryOperator(hasOperatorName("<"),
+                                    hasLHS(callExpr(callee(functionDecl(
+                                        matchesName("^::posix_"),
+                                        unless(hasName("::posix_openpt")))))),
+                                    hasRHS(integerLiteral(equals(0))))
+                         .bind("ltzop"),
+                     this);
+  Finder->addMatcher(binaryOperator(hasOperatorName(">="),
+                                    hasLHS(callExpr(callee(functionDecl(
+                                        matchesName("^::posix_"),
+                                        unless(hasName("::posix_openpt")))))),
+                                    hasRHS(integerLiteral(equals(0))))
+                         .bind("atop"),
+                     this);
+  Finder->addMatcher(
+      binaryOperator(
+          anyOf(hasOperatorName("=="), hasOperatorName("!="),
+                hasOperatorName("<="), hasOperatorName("<")),
+          hasLHS(callExpr(callee(functionDecl(
+              matchesName("^::posix_"), unless(hasName("::posix_openpt")))))),
+          hasRHS(unaryOperator(hasOperatorName("-"),
+                               hasUnaryOperand(integerLiteral()))))
+          .bind("binop"),
+      this);
+}
+
+void PosixReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *LessThanZeroOp =
+          Result.Nodes.getNodeAs<BinaryOperator>("ltzop")) {
+    SourceLocation OperatorLoc = LessThanZeroOp->getOperatorLoc();
+    diag(OperatorLoc, "the comparison always evaluates to false because %0 "
+                      "always returns non-negative values")
+        << getFunctionSpelling(Result, "ltzop")
+        << FixItHint::CreateReplacement(OperatorLoc, Twine(">").str());
+    return;
+  }
+  if (const auto *AlwaysTrueOp =
+          Result.Nodes.getNodeAs<BinaryOperator>("atop")) {
+    diag(AlwaysTrueOp->getOperatorLoc(),
+         "the comparison always evaluates to true because %0 always returns "
+         "non-negative values")
+        << getFunctionSpelling(Result, "atop");
+    return;
+  }
+  const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  diag(BinOp->getOperatorLoc(), "%0 only returns non-negative values")
+      << getFunctionSpelling(Result, "binop");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.h

@@ -0,0 +1,30 @@
+//===--- PosixReturnCheck.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_BUGPRONE_POSIX_RETURN_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POSIX_RETURN_CHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+class PosixReturnCheck: public ClangTidyCheck {
+public:
+  PosixReturnCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POSIX_RETURN_CHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -125,6 +125,12 @@ Improvements to clang-tidy
   repeated branches in ``switch`` statements and indentical true and false
   branches in conditional operators.
 
+- New :doc:`bugprone-posix-return
+  <clang-tidy/checks/android-posix-return>` check.
+
+  Checks if any calls to POSIX functions (except ``posix_openpt``) expect negative
+  return values.
+
 - New :doc:`fuchsia-default-arguments-calls
   <clang-tidy/checks/fuchsia-default-arguments-calls>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst

@@ -0,0 +1,20 @@
+.. title:: clang-tidy - bugprone-posix-return
+
+bugprone-posix-return
+=====================
+
+Checks if any calls to POSIX functions (except ``posix_openpt``) expect negative
+return values. These functions return either ``0`` on success or an ``errno`` on failure,
+which is positive only.
+
+Example buggy usage looks like:
+
+.. code-block:: c
+
+  if (posix_fadvise(...) < 0) {
+
+This will never happen as the return value is always non-negative. A simple fix could be:
+
+.. code-block:: c
+
+  if (posix_fadvise(...) > 0) {

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -58,6 +58,7 @@ Clang-Tidy Checks
    bugprone-move-forwarding-reference
    bugprone-multiple-statement-macro
    bugprone-parent-virtual-call
+   bugprone-posix-return
    bugprone-sizeof-container
    bugprone-sizeof-expression
    bugprone-string-constructor

clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp

@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s bugprone-posix-return %t
+
+#define NULL nullptr
+#define ZERO 0
+#define NEGATIVE_ONE -1
+
+typedef long off_t;
+typedef decltype(sizeof(int)) size_t;
+typedef int pid_t;
+typedef struct __posix_spawn_file_actions* posix_spawn_file_actions_t;
+typedef struct __posix_spawnattr* posix_spawnattr_t;
+
+extern "C" int posix_fadvise(int fd, off_t offset, off_t len, int advice);
+extern "C" int posix_fallocate(int fd, off_t offset, off_t len);
+extern "C" int posix_madvise(void *addr, size_t len, int advice);
+extern "C" int posix_memalign(void **memptr, size_t alignment, size_t size);
+extern "C" int posix_openpt(int flags);
+extern "C" int posix_spawn(pid_t *pid, const char *path,
+                const posix_spawn_file_actions_t *file_actions,
+                const posix_spawnattr_t *attrp,
+                char *const argv[], char *const envp[]);
+extern "C" int posix_spawnp(pid_t *pid, const char *file,
+                 const posix_spawn_file_actions_t *file_actions,
+                 const posix_spawnattr_t *attrp,
+                 char *const argv[], char *const envp[]);
+
+void warningLessThanZero() {
+  if (posix_fadvise(0, 0, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: the comparison always evaluates to false because posix_fadvise always returns non-negative values
+  // CHECK-FIXES: posix_fadvise(0, 0, 0, 0) > 0
+  if (posix_fallocate(0, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning:
+  // CHECK-FIXES: posix_fallocate(0, 0, 0) > 0
+  if (posix_madvise(NULL, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  // CHECK-FIXES: posix_madvise(NULL, 0, 0) > 0
+  if (posix_memalign(NULL, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning:
+  // CHECK-FIXES: posix_memalign(NULL, 0, 0) > 0
+  if (posix_spawn(NULL, NULL, NULL, NULL, {NULL}, {NULL}) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:59: warning:
+  // CHECK-FIXES: posix_spawn(NULL, NULL, NULL, NULL, {NULL}, {NULL}) > 0
+  if (posix_spawnp(NULL, NULL, NULL, NULL, {NULL}, {NULL}) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:60: warning:
+  // CHECK-FIXES: posix_spawnp(NULL, NULL, NULL, NULL, {NULL}, {NULL}) > 0
+}
+
+void warningAlwaysTrue() {
+  if (posix_fadvise(0, 0, 0, 0) >= 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: the comparison always evaluates to true because posix_fadvise always returns non-negative values
+}
+
+void warningEqualsNegative() {
+  if (posix_fadvise(0, 0, 0, 0) == -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: posix_fadvise
+  if (posix_fadvise(0, 0, 0, 0) != -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fadvise(0, 0, 0, 0) <= -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fadvise(0, 0, 0, 0) < -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fallocate(0, 0, 0) == -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning:
+  if (posix_madvise(NULL, 0, 0) == -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_memalign(NULL, 0, 0) == -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning:
+  if (posix_spawn(NULL, NULL, NULL, NULL, {NULL}, {NULL}) == -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:59: warning:
+  if (posix_spawnp(NULL, NULL, NULL, NULL, {NULL}, {NULL}) == -1) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:60: warning:
+}
+
+void WarningWithMacro() {
+  if (posix_fadvise(0, 0, 0, 0) < ZERO) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  // CHECK-FIXES: posix_fadvise(0, 0, 0, 0) > ZERO
+  if (posix_fadvise(0, 0, 0, 0) >= ZERO) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fadvise(0, 0, 0, 0) == NEGATIVE_ONE) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fadvise(0, 0, 0, 0) != NEGATIVE_ONE) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fadvise(0, 0, 0, 0) <= NEGATIVE_ONE) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fadvise(0, 0, 0, 0) < NEGATIVE_ONE) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+}
+
+void noWarning() {
+  if (posix_openpt(0) < 0) {}
+  if (posix_openpt(0) <= 0) {}
+  if (posix_openpt(0) == -1) {}
+  if (posix_openpt(0) != -1) {}
+  if (posix_openpt(0) <= -1) {}
+  if (posix_openpt(0) < -1) {}
+  if (posix_fadvise(0, 0, 0, 0) <= 0) {}
+  if (posix_fadvise(0, 0, 0, 0) == 1) {}
+}
+
+namespace i {
+int posix_fadvise(int fd, off_t offset, off_t len, int advice);
+
+void noWarning() {
+  if (posix_fadvise(0, 0, 0, 0) < 0) {}
+  if (posix_fadvise(0, 0, 0, 0) >= 0) {}
+  if (posix_fadvise(0, 0, 0, 0) == -1) {}
+  if (posix_fadvise(0, 0, 0, 0) != -1) {}
+  if (posix_fadvise(0, 0, 0, 0) <= -1) {}
+  if (posix_fadvise(0, 0, 0, 0) < -1) {}
+}
+
+} // namespace i
+
+class G {
+ public:
+  int posix_fadvise(int fd, off_t offset, off_t len, int advice);
+
+  void noWarning() {
+    if (posix_fadvise(0, 0, 0, 0) < 0) {}
+    if (posix_fadvise(0, 0, 0, 0) >= 0) {}
+    if (posix_fadvise(0, 0, 0, 0) == -1) {}
+    if (posix_fadvise(0, 0, 0, 0) != -1) {}
+    if (posix_fadvise(0, 0, 0, 0) <= -1) {}
+    if (posix_fadvise(0, 0, 0, 0) < -1) {}
+  }
+};