[PATCH] D24507: Add attribute for return values that shouldn't be cast to bool

[Anton Urusov via cfe-commits]

urusant created this revision.
urusant added reviewers: zaks.anna, dcoughlin, jordan_rose, NoQ.
urusant added subscribers: cfe-commits, daviddrysdale.
urusant set the repository for this revision to rL LLVM.
Herald added subscribers: mgorny, beanz.

Hi,

I am interested in feedback on a patch I was working on that adds a new attribute (warn_impcast_to_bool) to indicate that the return value of a function shouldn't be used as a boolean, as well as a compile warning and a StaticAnalyzer checker to warn about misusing functions with this attribute. I'd also appreciate any suggestions for how to deal with a class of false positives that the static analysis checker produces.

The change was originally inspired by CVE-2008-5077 [1], which was the result of an odd design choice in OpenSSL: having an API that returns 1 for success, 0 for failure... and -1 for catastrophic failure. Various API users fell into the trap of treating the return value as a boolean, so the patch adds an attribute to allow this to trigger a warning.

As well as generating a compile-time warning, the patch also includes a new static analyzer checker to catch more indirect uses, where the non-boolean integer value gets propagated via function wrappers or local variables. However, it gives a false positive for cases when the code using the return value actually checks the value in a non-boolean way (because the SVal doesn't reflect the fact that the value has been further constrained). I couldn't see an obvious way to get anything relevant from the RangeConstraintManager; any suggestions?

To test the check (beyond the included unit tests), I annotated dangerous OpenSSL functions and tried building 8 OpenSSL-using codebases with it. So far, this didn't give many results for them: the only possible problem was found in ruby2.1, which was already fixed a few months ago. However, this change is still potentially useful - even 7 years after the original CVE, there are still codebases that fall into OpenSSL's API trap.

[1] https://www.openssl.org/news/secadv/20090107.txt

Repository:
  rL LLVM

https://reviews.llvm.org/D24507

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
  test/ReturnNonBoolTest.c
  test/ReturnNonBoolTest.cpp
  test/ReturnNonBoolTestCompileTime.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D24507.71157.patch
Type: text/x-patch
Size: 20120 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160913/5ba9e03c/attachment-0001.bin>