Page MenuHomePhabricator

[clang][checkers] Added new checker 'error-return-checker'.
Needs ReviewPublic

Authored by balazske on Dec 14 2019, 2:25 AM.

Details

Summary

Adding a new checker alpha.unix.ErrorReturn.

This check should implement SEI CERT C Coding Standard rule
"ERR33-C. Detect and handle standard library errors".

Event Timeline

balazske created this revision.Dec 14 2019, 2:25 AM

Code is to be reformatted later.

balazske updated this revision to Diff 234730.Dec 19 2019, 8:40 AM

Adding a new diff over the previous one.
(The commit was amended accidentally.)

balazske updated this revision to Diff 236070.Fri, Jan 3, 8:17 AM
  • Implemented all (now possible) functions.
  • Moved ParmVal value to own state map.
  • Improved state data and bug reporting.

Works relatively good now but not perfect. The tests are sometimes too strict so there are some false positives, for example this case:

unsigned long X = strtoul("345", NULL, 10);
if (X > 100) {
 // handle error
}

The result is not checked for ULONG_MAX but still the code is correct in this way. But we can not figure out the intention of the programmer to detect what is an "error handling code" to check for error handling branches. Other solution is to detect any branch condition that involves the value (returned from the function) as a "test for error return value" but this is probably not better.

balazske updated this revision to Diff 236349.Mon, Jan 6, 6:20 AM
  • Added variadic functions, improved comments.

Great job, this seems to be progressing nicely! please see my comments inline.

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
39

Value is not the name of the parameter, maybe a refactoring missed this comment.

78

I think the current implementation is slightly different. The ValueErrorResultChecker uses symbolic evaluation of the equation with a ConcreteInt 0 value, while the ProgramState::isNull checks if the value is a constant and if it is not a zero constant (also calls further to ConstraintManager::isNull).

159

Maybe only checking for negative value is enough? My gut feeling is that the equality check is redundant. I would argue:
let A be: Value is negative
let B be: Value is equal to -1

since B implies A, (A or B) can be simplified to just A.
This presupposes that every time B can reasoned about A can be too.
This seems to be the case for here as well, as the definiteness of being equal to -1 is stronger than being lower than 0.

clang/test/Analysis/error-return.c
271

just for the sake of completeness; mention all the cases where error checking is deemed unnecessary as per ERR-33-EX1

https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors#ERR33-C.Detectandhandlestandardlibraryerrors-Exceptions

balazske marked 3 inline comments as done.Tue, Jan 7, 12:07 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
78

I think if there is a method for a special purpose (isNull) it is better to use that, it can be more efficient.

159

The test

void test_NegativeOrEofCorrectCheck1() {
  int X = fputs("", NULL);
  if (X == -1) {
  }
}

fails when only the check for negative value is there.

The problem is with the negated part: If there is a "X==-1" statement in the code to check then the "X<0" assumption is true but "X>=0" is not known because X can be less than -1. This check should accept the "X==-1" and "X<0" type of code.

clang/test/Analysis/error-return.c
271

It seems that the better solution is to check for those functions too, and add a feature to the checker that ignores functions that are casted to void. The exception list says that even in these exceptions the (void) cast is needed. (Probably a other kind of warning message can be used for these functions.) The "function can not fail" and "return value is inconsequential" cases are not detectable by the checker (these cases depend on the context of the function call, not on the function itself, otherwise the function should appear in the list of functions to be ignored).

balazske updated this revision to Diff 237053.Thu, Jan 9, 6:19 AM
  • Prevent warning if call is in a return statement.
  • Prevent warning if call is casted to void.
  • Added documentation.
  • Fixed the tests.
  • Some other fixes.
balazske retitled this revision from [clang][checkers] Added new checker 'error-return-checker'. (WIP) to [clang][checkers] Added new checker 'error-return-checker'..Thu, Jan 9, 6:31 AM
balazske edited the summary of this revision. (Show Details)
balazske updated this revision to Diff 237061.Thu, Jan 9, 6:41 AM
  • More fixes in test files.

Checker was tested with tmux, no problems found (there are false positives but not easy to fix).

balazske updated this revision to Diff 237680.Mon, Jan 13, 7:51 AM
  • Improved function list format in documentation.
NoQ added a comment.Mon, Jan 13, 5:05 PM

Uh-oh, what happened here?

Please don't post huge patches. 600 lines of code is huge. You could start off by implementing a single check (eg., NullErrorResultChecker) with a single library function and then add more checks and more functions in follow-up patches; this would have been much easier to review and would have made the community happy.


Ok, here's how i see this:

  1. In the beginning there was __attribute__((warn_unused_result)). It came with a compiler warning when the return value was completely discarded. Putting it into a variable or casting it to (void) silenced the warning.
  2. Then someone decided that putting that attribute on printf and scanf is not humane because it will infuriate all first-year computer science students who just want to read a number, multiply it by 2 and print the answer. So it was removed from most functions that needed it for security-related reasons, and only remained on, basically, pure functions such as std::vector::empty() (so that it wasn't confused with std::vector::clear()).
  3. Removing the attribute from library headers means that the compiler will be completely unable to diagnose upon discarded return values. After all, the compiler isn't allowed to possess knowledge of APIs.
  4. This is where @balazske comes in and decides to put the extra warning into the Static Analyzer. After all, it's allowed to possess knowledge of APIs, and it's also an opt-in tool, which avoids the first-year students problem.
  5. But then @balazske becomes obsessed with the power that the Static Analyzer gives him and decides to also make the warning perfectly precise with the help of path-sensitive analysis. This would show it to all the nasty developers who have so far got away with casting the value to (void) to suppress the warning. They can no longer trick us. They will have to actually check the value.

I want to discuss step 5. Do we really need to go that far? Your solution is mathematically perfect (it probably isn't just by looking at the set of the checker callbacks that you've subscribed so far, but suppose it is). But is it actually good for the humanity to have the perfect solution? Do you really want to uncover all the places where the developer has intentionally suppressed the warning for the unused result? Do you really want to engage in an arms race with a developer who wants to silence the warning? Or would everybody be much happier if you simply re-used the implementation of __attribute__((warn_unused_result)) and treat its limitations as if it's the intended behavior?

Like, i myself don't have an answer to this question. I kind of understand both sides. Your approach is more costly, as you'll have to re-investigate all the previous suppression sites, but it may uncover a few more bugs, as well as some accidental omissions that were accidentally suppressed (e.g., the return value was intentionally put into a variable, but the value was never read (which is btw also a dead store, so our other checker will find it)). On the other hand, I believe that a simple AST-based approach employed by warn_unused_result would be good enough for most practical purposes, it would have low false positive rate (something we really value a lot) and provide an intuitive suppression mechanism, and it will also be much easier to maintain (which really pleases me as a maintainer who'll have to fix bugs in your code once it's committed, but ideally it shouldn't outweigh the benefits of the user).

I'd love to have you collect some data on this subject, and you're in a good position to do so, given that you already have a checker. If you run your checker on a lot of code, how many warnings will be non-trivially path-sensitive (i.e., the value isn't immediately discarded but dragged around for a bit, but still not checked)? Are these warnings valuable? How many of them highlight intentional suppressions?

balazske added a comment.EditedTue, Jan 14, 1:51 AM

The checker was implemented in smaller parts that are visible in the commit list. I can split the patch at the commits (and figure out how to use git history manipulations and phabricator in a better way?).
But not the line count in itself matters. Many lines are relatively "equivalent" in the FnInfo table and the tests (still these are something to look for).

I wanted to implement the rules described here:
https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors
This lists the functions to check so this knowledge has to be built into the checker and there are examples of this in other checkers (stream or memory or string functions for example). The functions have different kinds conditions on the return value in case of error. It is not sufficient to simply check that the return value is assigned to something or used because it may be used without check for error. So I tried to do something that can find if exactly a check for the error condition was made. (Still it is not perfect because the value can be used before the error checking.)
The cast to void is the special way to avoid the warning, in every other case the return value has to be checked for error or there is an exception that should be documented (if the function "can not fail" or the error does not matter). This may be too much warnings for a normal project but acceptable if extra safe code is needed. If we want to have that clang SA can (at least partially) check for that rule "ERR33-C" this (or similar) check is needed.