[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

There is a clang-tidy check bugprone-unused-return-value 

 that may be confusing in relation to this checker. It does similar job for a 
selected set of functions and with different reason.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> but it is planned that the checker takes use of the planned new attributes to 
> determine if a function should be checked

This is literally `__attribute__((warn_unused_result))` for which there already 
is a compiler warning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Great job!
I'd like to see more tests with the following cases:

- more expressions with function calls - ternary expression, arithmetic, array 
literals, initializer lists, etc.
- using/not using it in `if` statement with initializer
- using/not using it in different parts of GNU statement expressions
- calls in different parts of exception handling (both C++ and Obj-C)
- calls in lambdas/blocks/coroutines
- calls in different parts of C++ range-for loop
- calls in different parts of Obj-C for loop over collections
- calls under labels
- calls in attributed statements (e.g. `[[clang::nomerge]] foo()`)
- calls in goto statements
- calls in Obj-C `@synchronized` and `@autoreleasepool` statements


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added reviewers: NoQ, vsavchenko.
Szelethus added a comment.

Looks like a neat checker! I guess the only question is the function matching, 
and I don't dislike it in its current state. @martong, do you have any thoughts 
on this?

On another note, have you checked this on some projects yet? I expect that we 
might need to tone down on some of the functions if this ends up being too 
noisy.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:66-67
+continue;
+  if (!FD->isInStdNamespace() && !isInNoNamespace(FD))
+continue;
+

I suppose we need this to compensate for the fact that we can't reliably match 
on standard C functions.  Unfortunately, using `CallDescription` wouldn't 
necessarily solve this: D81745.



Comment at: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:85
+private:
+  llvm::StringMap FunctionsToCheck = {
+  {"aligned_alloc", 2}, {"asctime_s", 3}, {"at_quick_exit", 1},

balazske wrote:
> baloghadamsoftware wrote:
> > Hmm, why `StringMap<>`? Why not `CallDescriptionMap<>`?
> `CallDescriptionMap` is only usable with `CallEvent` that is not used in this 
> checker. Or it can be extended with lookup from `FunctionDecl`?
Good point. @martong, you have used `FunctionDecl` based checking, do you have 
any thoughts here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked an inline comment as done.
balazske added a comment.

Maybe it is better to have this check for every system function that has 
non-void return value (except specific functions)? This check is applicable to 
CERT rule ERR33-C, POS54-C and EXP12-C too (it is restricted but can find a 
part of the problems).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 2 inline comments as done.
balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:41
+BugReporter &BR) const {
+auto FoundCall = callExpr().bind("call");
+auto CallInCompound = compoundStmt(forEach(FoundCall));

baloghadamsoftware wrote:
> Please note that the `CallExpr` does not necessarily stands alone. It may be 
> wrapped into an `ExprWithCleanUps`. We should consider these `CallExpr`s as 
> unchecked too.
It looks like that the matcher finds these occurrences too. A test was added 
for it.



Comment at: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:85
+private:
+  llvm::StringMap FunctionsToCheck = {
+  {"aligned_alloc", 2}, {"asctime_s", 3}, {"at_quick_exit", 1},

baloghadamsoftware wrote:
> Hmm, why `StringMap<>`? Why not `CallDescriptionMap<>`?
`CallDescriptionMap` is only usable with `CallEvent` that is not used in this 
checker. Or it can be extended with lookup from `FunctionDecl`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 303444.
balazske added a comment.

Small fixes in test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/unchecked-return-value.cpp

Index: clang/test/Analysis/unchecked-return-value.cpp
===
--- /dev/null
+++ clang/test/Analysis/unchecked-return-value.cpp
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=security.UncheckedReturnValue -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+#include "Inputs/system-header-simulator.h"
+
+extern void extern_f(int);
+int btowc(int);
+
+struct S {
+  int getInt() const;
+};
+
+int f1(int X) {
+  scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  std::scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  other_ns::scanf("%*d");
+  {
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  if (X) {
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  if (X)
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  if (scanf("%*d")) {
+  }
+  if (scanf("%*d") > 1) {
+  }
+
+  while (true)
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  while (scanf("%*d")) {
+  }
+
+  do
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  while (true);
+  do {
+  } while (scanf("%*d"));
+
+  for (int Y = 1; Y < 10; ++Y)
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  for (int Y = 1; Y < 10; scanf("%*d")) { // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+  for (int Y = 1; scanf("%*d"); ++Y) {
+  }
+  int Y = 0;
+  for (scanf("%*d"); Y < 10; ++Y) { // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  std::vector Vec;
+  for (int I : Vec)
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+
+  switch (scanf("%*d")) {
+  }
+  switch (X) {
+  case 1:
+scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+break;
+  }
+
+  // This call is wrapped in an ExprWithCleanups.
+  scanf("", S().getInt()); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+
+  int Z1 = scanf("%*d");
+  Z1 = scanf("%*d");
+  extern_f(scanf("%*d"));
+
+  btowc(22); // no warning for non-system function
+
+  return scanf("%*d"); // no warning if value is returned
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1139,4 +1139,10 @@
   // TODO: Add some actual implementation.
 };
 
+int scanf(const char *format, ...);
+
 } // namespace std
+
+namespace other_ns {
+int scanf(const char *format, ...);
+}
Index: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp
@@ -0,0 +1,178 @@
+//===- ReturnValueChecker - Applies guaranteed return values *- 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
+//
+//===--===//
+//
+// This defines UncheckedReturnValueChecker, which checks for calls to functions
+// whose return value has to be observed by the program but is ignored.
+//
+//===--===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/StaticAnalyzer/Checkers/

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:41
+BugReporter &BR) const {
+auto FoundCall = callExpr().bind("call");
+auto CallInCompound = compoundStmt(forEach(FoundCall));

Please note that the `CallExpr` does not necessarily stands alone. It may be 
wrapped into an `ExprWithCleanUps`. We should consider these `CallExpr`s as 
unchecked too.



Comment at: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:85
+private:
+  llvm::StringMap FunctionsToCheck = {
+  {"aligned_alloc", 2}, {"asctime_s", 3}, {"at_quick_exit", 1},

Hmm, why `StringMap<>`? Why not `CallDescriptionMap<>`?



Comment at: clang/test/Analysis/unchecked-return-value.cpp:10
+int f1(int X) {
+  scanf(""); // expected-warning {{Return value is not checked in call to 
'scanf' [security.UncheckedReturnValue]}}
+  std::scanf(""); // expected-warning {{Return value is not checked in call to 
'scanf' [security.UncheckedReturnValue]}}

Please use some valid format here. E.g. `scanf("%*c");`



Comment at: clang/test/Analysis/unchecked-return-value.cpp:16
+scanf(""); // expected-warning {{Return value is not checked in call to 
'scanf' [security.UncheckedReturnValue]}}
+  }
+

Please add such simple test case for all the functions we try to check. (One 
call is enough for every such function, either in `std::` or on the top 
namespace.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

This is a very simplified form of the check that is implemented in D72705 
 but still may be useful in practical cases. 
This check could be a clang-tidy check but it is planned that the checker takes 
use of the planned new attributes to determine if a function should be checked.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90691/new/

https://reviews.llvm.org/D90691

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: cfe-commits, steakhal, ASDenysPetrov, martong, 
Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, 
szepet, baloghadamsoftware, xazax.hun, whisperity, mgorny.
Herald added a reviewer: Szelethus.
Herald added a project: clang.
balazske requested review of this revision.

Add a checker that checks if a function is used without examining
its return value. The set of functions is taken from CERT rule
ERR33-C "Detect and handle standard library errors". This is a
simplified check for the problem.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90691

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/unchecked-return-value.cpp

Index: clang/test/Analysis/unchecked-return-value.cpp
===
--- /dev/null
+++ clang/test/Analysis/unchecked-return-value.cpp
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=security.UncheckedReturnValue -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+#include "Inputs/system-header-simulator.h"
+
+extern void extern_f(int);
+int btowc(int);
+
+int f1(int X) {
+  scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  std::scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  other_ns::scanf("");
+  {
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  if (X) {
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  if (X)
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  if (scanf("")) {
+  }
+  if (scanf("") > 1) {
+  }
+
+  while (true)
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  while (scanf("")) {
+  }
+
+  do
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  while (true);
+  do {
+  } while (scanf(""));
+
+  for (int Y = 1; Y < 10; ++Y)
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  for (int Y = 1; Y < 10; scanf("")) { // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+  for (int Y = 1; scanf(""); ++Y) {
+  }
+  int Y = 0;
+  for (scanf(""); Y < 10; ++Y) { // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  std::vector Vec;
+  for (int I : Vec)
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+
+  switch (scanf("")) {
+  }
+  switch (X) {
+  case 1:
+scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+break;
+  }
+
+  int Z1 = scanf("");
+  Z1 = scanf("");
+  extern_f(scanf(""));
+
+  btowc(22); // no warning for non-system function
+
+  return scanf("");
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1139,4 +1139,10 @@
   // TODO: Add some actual implementation.
 };
 
+int scanf(const char *format, ...);
+
 } // namespace std
+
+namespace other_ns {
+int scanf(const char *format, ...);
+}
Index: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp
@@ -0,0 +1,178 @@
+//===- ReturnValueChecker - Applies guaranteed return values *- 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
+//
+//===--===//
+//
+// This defines UncheckedReturnValueChecker, which checks for calls to functions
+// whose return value has to be observed by the program but is ignored.
+//
+//===--===//
+
+#include "clang/ASTMatchers/ASTMatchFind