Page MenuHomePhabricator

[analyzer] Added new checker 'alpha.unix.ErrorReturn'.
Needs RevisionPublic

Authored by balazske on Jan 14 2020, 7:49 AM.

Details

Summary

First simple implementation, infrastructure is ready.
Only one kind of check (NULL error return value) is implemented.
Documentation not added yet.
Idea of the checker is taken from:
https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

If more discussion is needed it is better to use "Discourse" instead of this review?

I've been meaning to suggest that we use either that or discord for casual chatting (for simple questions etc), but given that we already have a working prototype, I think it'd be better to keep the conversation here for now.

balazske updated this revision to Diff 243178.Feb 7 2020, 8:27 AM

Adding a simplified version.

  • The check is only for "EOFOrNegative" case.
  • Long list of functions is removed.
  • Not checking for "access in condition".
  • Removed the warning if value is unused.
balazske updated this revision to Diff 243758.Feb 11 2020, 1:07 AM
balazske marked 2 inline comments as done.

Improved tests.

One of the things that stood out for me was the lack of the usage of the check::BranchCondition callback, but there you'd have to grind out whether it is relevant to a return value, so I'm probably wrong on that regard.

So I guess I don't have any immediate high level objections. Using a recursive statement visitor seems overkill, but maybe its appropriate here, and lets leave that discussion for later anyways. Overriding CheckerBase::printState to show the current set of values stored in CalledFunctionDataMap would be nice sometime, but that can wait as well.

@NoQ, @xazax.hun, @baloghadamsoftware, how do you like this patch? I think the high level idea is correct.

Also, allow me to add a few other folks, because they are very active and knowledgeable individuals :)

I think this is a useful concept and it can nicely complement the ongoing summary based work in StdLibraryFunctionsChecker. Still, in my opinion, there are more discussions and work to be done here before we get to the final implementation.

Our analyzer does not reason about all paths, we try to find only ONE path where an error occurs. This leads to a non-trivial question: Is it an error if we have some paths that do not check for the return value and at least one path where we do?
(1) One answer could be that there is bug if we find out that we use a rotten file descriptor or a nullptr. But, if we don't "use" them then that is not bug. How do we define "use"? We could say that any function that takes the return value is using that, but that may lead to false positives if we cannot see into the function body (that function may check the value). I think we should define "use" in the terms of preconditions of other functions. In the previous example (use(*P)) we should be able to specify that the parameter of use must not be null and if that condition is violated only then should we diagnose the bug. In the CERT description we see that the different functions are related. For instance, we should use fgetc only with a valid FILE* that must not be NULL. This approach is being implemented in StdLibraryFunctionsChecker.

(2) On the other hand, there are cases when there is no such visible data dependency between the functions. E.g. the example from ERR33-C:

size_t read_at(FILE *file, long offset,
               void *buf, size_t nbytes) {
  fseek(file, offset, SEEK_SET);
  return fread(buf, 1, nbytes, file);
}

Here, there are no argument constraints to fread that may be violated by calling fseek! But, let's just remove the call of fread:

size_t read_at(FILE *file, long offset,
               void *buf, size_t nbytes) {
  return fseek(file, offset, SEEK_SET);
}

The bug has gone! I suggest that we should create/describe function groups. In a group we would define those functions that are related. E.g. fseek, fread, fopen, etc. If they operate on the same stream and we can find a path where there is an fseek and then an fread without the return value checked then that is a real bug. What do you think? Maybe we should have a generic infrastructure that could be reused in StreamChecker?

A problem with this approach is that it works only if the value (the return value of the function) is not constrained already after the function call (at least not for the error value), otherwise it will interpret (wrongly) this constrainment as result of an error check branch in the code. So it is not possible to make a branch with fixed error return value (if other checker do this this is probably bad too). And it is not possible (?) to make a new branch without constraint.
...
For example a postCall put constraints on it if it is known that the returned value is always non-negative. Or more worse, an error path is generated when the return value is exactly the error return code. In the mentioned case if the f function is modeled and a path is added where it has return value zero the checker will "think" that there was an if (X == 0) condition in the code.

In my understanding, if the return value is already constrained (to not have the error value) on a path that means we are "ready" on that path, i.e. we don't have to check again for the return value on that path. However, there may be other paths where we have to seek for the check.

Generally, is it a problem if there are multiple checkers that may detect same defects but with different approach? Or should the code of this checker (for example) made more complicated only to filter out cases that may be detected by other checkers too?

If we can avoid it then do it, but, in my opinion, that is not a problem. There will be one checker which finds the error first then emits an error node and then the analysis stops, the next checker will not find the same bug, because execution had already stopped. We need to define the order of the colliding checkers though. Actually, there is an example for such a case: D79420 (Note, I am not sure about non-fatal errors, does the analysis continue in that case?)

Probably we have different pre-conceptions about this checker. One thing is to discover the error by finding a violated precondition or other bug that is caused by the wrong return value of the called and failed function. This is case (1) above if I am correct. Other way is to discover the error by checking the state of an object that a function manipulates and find use of the object when it is in bad state. This is the case (2) above if I am correct. These are things that are done by other checkers partially already now. Case (1) is job of StdLibraryFunctionsChecker or other checkers, maybe any check for use of zero pointers. Case (2) is job of StreamChecker and similar checkers that maintain state of some object or "handle". For many of the functions in ERR33-C no such checker exists yet and it would be very much work do make all of these (or not possible at all).

This change is doing another approach: It checks directly the value that is returned from a function that may fail, not the object that the function manipulates. The goal is to check if the return value of a function is checked in the code. This may be discovered when the value is passed to a next function call and the precondition of that function is violated but this is not the goal of this checker. It tries to find an execution path where the returned value is used in the code in a way that is said to be an error check, or in a way that is an other use. The error check is discovered by checking the statement where the value appears to discover if it is compared against an error code (specific to the called function that produced the value).

It is a way of find some of the cases with the mentioned way of tracking what operations are allowed after what operations with or without error check, on a specific stream. Instead of a stream a symbol that is passed to the function at a specific argument can be used (to generalize for other than streams). If the code would work this way, the check to discover the "error check statement" is still needed, like now in the checker.

But I think it is no problem to say that the return value should be checked always (for the specific functions). The fact that a function has failed and the return value was not checked is in itself a bug and should not discovered only when a later function is called. This is not always possible. For example at strtol type of functions we can only find if the return value is checked for special error return value. A failing strtol may not cause any discoverable abnormal conditions later.

It is a way of find some of the cases with the mentioned way of tracking what operations are allowed after what operations with or without error check, on a specific stream. Instead of a stream a symbol that is passed to the function at a specific argument can be used (to generalize for other than streams). If the code would work this way, the check to discover the "error check statement" is still needed, like now in the checker.

But I think it is no problem to say that the return value should be checked always (for the specific functions). The fact that a function has failed and the return value was not checked is in itself a bug and should not discovered only when a later function is called. This is not always possible. For example at strtol type of functions we can only find if the return value is checked for special error return value. A failing strtol may not cause any discoverable abnormal conditions later.

Yes, absolutely. It makes sense what you suggest. I am still a bit worried about false positives. Nevertheless, this is absolutely a good work with added value. I suggest to move on and do some analysis on real projects with this checker (tmux, curl, redis, xerces, bitcoin, ...). And see if it finds any new bugs and try to find out whether they are false positives or not. Also, would be interesting to see if there is any performance regression in the overall analysis time (I am a bit concerned about checkLocation).

balazske marked an inline comment as done.May 18 2020, 8:32 AM

I can make comparisons if more functions are added to the checker, fputs only is not sufficient.

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

fix ErrorReturnCHecker here

I can make comparisons if more functions are added to the checker, fputs only is not sufficient.

Yes, you cannot measure just with fputs, similar thing happened to me with StdCLibraryFunctions, thus I had to add many many more functions to be able to measure.
So, what if you had a private branch where you have several functions other than fputs added. If I were you I'd search (count) for the interesting functions (the ones mentioned in ERR33) in one of the open projects: e.g. tmux. If there is reasonable number of such functions, then I'd sort these functions by the number of calls on them. I'd implement the ones with the highest count and smallest effort needed. Then we'd have some numbers to bolster this direction.

After running the checker I could observe the following problems:

time_t now = time(NULL);
if (now > 0) { ... }

Here only now == EOF would be correct for the checker, so this case is reported (false positive). It may be better if the checker finds any "now > x" where x is non-negative. This can be used for any function that returns an integer value (not pointer) and EOF is the error return code.

c = fgetc(fd);
if (c == '+' || c == '*' || c == '|' || c == '>' || c == '@' || c == EOF || c == '\n') { ... }

The first c == '+' is found by the checker and reported as false positive (the later c == EOF is not found). Such a case can be found if the checker can collect expressions that are separated by || or && and the symbol to check occurs in these and there is only a simple comparison.

The checker can find places where the return value is tested for error (mostly early-return cases), not where the return value is tested for a valid value (that may be a subset of all non-error values). And the test for error or valid value should be in a single statement, not in nested ifs for example.

NoQ added a comment.Jun 11 2020, 1:10 PM

@NoQ, @xazax.hun, @baloghadamsoftware, how do you like this patch? I think the high level idea is correct.

I still don't understand how do we enable this checker, i.e. for whom and how often. Is there anything we'll be able to turn on by default, like maybe warn on all functions that wear __attribute__((warn_unused_result))?

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
310–312

Why scan parent statements proactively given that we will reach them later anyway? Looks like you're using a wrong checker callback.

NoQ added a comment.Jun 16 2020, 3:08 AM

Sounds like we have a feature request! https://bugs.llvm.org/show_bug.cgi?id=46346

Szelethus added inline comments.Jun 24 2020, 8:47 AM
clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
310–312

Thats one of the things I'm not super familiar with just yet. Are you referencing how a CFGBlock is built up? I remember trying to solve a similar problem (dig writes/reads out of Stmt *s) when working on the reaching definitions calculator, and I question my methodology as well.

test.cpp:

int getInt();

void different_assignments() {
  int i = getInt(); // Return value to check.

  if (static_cast<bool>(i < 7))
    ;
}

AST:

`-FunctionDecl 0xaf6ff0 <line:3:1, line:8:1> line:3:6 different_assignments 'void ()'
  `-CompoundStmt 0xaf72d8 <col:30, line:8:1>
    |-DeclStmt 0xaf71e8 <line:4:3, col:19>
    | `-VarDecl 0xaf70a8 <col:3, col:18> col:7 used i 'int' cinit
    |   `-CallExpr 0xaf71c8 <col:11, col:18> 'int'
    |     `-ImplicitCastExpr 0xaf71b0 <col:11> 'int (*)()' <FunctionToPointerDecay>
    |       `-DeclRefExpr 0xaf7158 <col:11> 'int ()' lvalue Function 0xaf6eb8 'getInt' 'int ()'
    `-IfStmt 0xaf72c0 <line:6:3, line:7:5>
      |-CXXStaticCastExpr 0xaf7288 <line:6:7, col:30> 'bool' static_cast<_Bool> <NoOp>
      | `-BinaryOperator 0xaf7258 <col:25, col:29> 'bool' '<'
      |   |-ImplicitCastExpr 0xaf7240 <col:25> 'int' <LValueToRValue>
      |   | `-DeclRefExpr 0xaf7200 <col:25> 'int' lvalue Var 0xaf70a8 'i' 'int'
      |   `-IntegerLiteral 0xaf7220 <col:29> 'int' 7
      `-NullStmt 0xaf72b8 <line:7:5>

CFG:

void different_assignments()
 [B3 (ENTRY)]
   Succs (1): B2

 [B1]
   Preds (1): B2
   Succs (1): B0

 [B2]
   1: getInt
   2: [B2.1] (ImplicitCastExpr, FunctionToPointerDecay, int (*)(void))
   3: [B2.2]()
   4: int i = getInt();
   5: i
   6: [B2.5] (ImplicitCastExpr, LValueToRValue, int)
   7: 7
   8: [B2.6] < [B2.7]
   9: static_cast<bool>([B2.8]) (CXXStaticCastExpr, NoOp, _Bool)
   T: if [B2.9]
   Preds (1): B3
   Succs (2): B1 B0

 [B0 (EXIT)]
   Preds (2): B1 B2

I can see that a single statement has multiple CFGElements associated with it (implying that multiple checker callbacks might be invoked). Is your point tat we shouldn't do all the analysis at Block 2 CFGElement 6, but rather at Block 2 Element 9?

As I see this checker, what we want at the end of the day is to see a critical return value (say ptr) or a derived value of it (say (ptr != nullptr && someOtherPrecondition()) in a branch condition. But I think working from checkBranchCondition and working down the AST (does ptr or some other return value participate in this condition?) , as opposed to using checkLoation and working working up (is ptr's read here a part of a condition?) doesn't immediately feel like an easier, shorter or less error-prone solution.

A random idea, what we could try out is to store accesses to return values in checkLocation in the GDM, and check in checkBranchCondition whether its a substatement.

The original plan was to enable the check for the functions listed in ERR33-C. But there are many other functions that work in similar way (like mmap that is not in the list but there is a request for similar check). The knowledge about what functions should be checked and how is built into the checker. Unless we find a way to specify this data in an external way, maybe as part of "summary" (if the summary contains extra information about what kind of return value indicates an error result). The current way of summary (StdLibraryFunctionsChecker) could be extended to make the summary data available for any checker.

Possible improvements (this is at least a note for me):

  • The checker can try to find a check for error-return value until the symbol (to check) is released. The warning is generated only if no check for error-return was found. This way works for functions that return a numerical value that has other meaningful values beside the error-return value. There is not necessary that the check for the error return is the first check. For other functions the first use of the return value must be the check for error-return. (What way to choose is a property of the system call to check.)
  • More kinds of error-return kinds can be added. For example: If a function has a numerical return value where -1 is error return, a check for "> x" could be accepted instead of check for "> -1".

After reading @martong's comment D72705#2035844 and @balazske's comment D72705#2086994, I think there is a need to argue across all paths of execution within the function, and as such this is a dataflow problem.

c = fgetc(fd);
if (c == '+' || c == '*' || c == '|' || c == '>' || c == '@' || c == EOF || c == '\n') { ... }

The first c == '+' is found by the checker and reported as false positive (the later c == EOF is not found). Such a case can be found if the checker can collect expressions that are separated by || or && and the symbol to check occurs in these and there is only a simple comparison.

Our current dataflow analyses definitely have to make judgement of which block (and in that, which CFGStmts) reads or writes a variable/expression. You can actually register an observer for expression writes (or in other terms, kills), so I don't think it'd be too challenging to do this with reads.

DeadStoresChecker is a great example for a checker that utilizes dataflow and pathsensitive analyses.

Generally, is it a problem if there are multiple checkers that may detect same defects but with different approach?

If avoidable, yes. If not, I think its an acceptable sacrifice.

Mind that since D80905 landed, the tone has shifted, if a more checker detects a specific bug (like double free) but a more general checker does as well (as passing garbage value to a function) we're okay with that.

We must check on every execution path that a specific condition on a value is satisfied (or: find one where the condition is not satisfied). This would be the most simple form of this checker. This can be done with the path sensitive check and checkDeadSymbols. If the symbol becomes dead with unsatisfied condition a warning is emitted. The "condition" is a syntactic-like check that looks for code like "X != EOF". This is probably done best by using StmtVisitor if a value is evaluated (at assignment or condition in if or other statements), different than the current form.

We must check on every execution path that a specific condition on a value is satisfied (or: find one where the condition is not satisfied). This would be the most simple form of this checker. This can be done with the path sensitive check and checkDeadSymbols. If the symbol becomes dead with unsatisfied condition a warning is emitted. The "condition" is a syntactic-like check that looks for code like "X != EOF". This is probably done best by using StmtVisitor if a value is evaluated (at assignment or condition in if or other statements), different than the current form.

Pathsensitivity is about checking one specific path of execution with rather great precision, we really need dataflow to argue about all paths. Syntactic checking and pathsensitive analysis fundamentally lacks a lot of information that dataflow by design has. With that said, asking whether a symbol is dead is a part of liveness analysis which is a dataflow algorithm, but what that interface lacks is why a symbol is live/dead. I think what you need here is a set of reads reachable from the return value point. For this example, you are interested in which reads are reachable from b9, and you could analyze them one-by-one (which could be done syntactically at that point):

c = fgetc(fd); // [b9]
if (c == '+' || c == '*' || c == '|' || c == '>' || c == '@' || c == EOF || c == '\n') { [b1] }
//    [b8]        [b7]        [b6]        [b5]        [b4]       [b3]         [b2] 
// [b0 (EXIT)]

The problem with fgetc is here that the returned value indicates not only if there was failure or not. In the following (over simplified) example no warning is needed even if there is no check for EOF. Because this case, for such kind of functions any comparison of the return value should be treated as an error check. Similar functions are the strtol family.

c = fgetc(fd);
if (c == 'a') {
  // this is the expected input
} else {
  // any other value is handled as error
}

I hope you don't mind if I bring up a point you mentioned during a daily meeting.

The error checking shouldn't be present on one of execution, but rather on all of them.

I think your recent comment highlights why that wouldn't be sufficient, unfortunately. Also, the else branch will have be accounted for, good forward thinking.

This checker can have two purposes, one is to verify that all paths have the error check, other is to find at least one path without error check. The first is the one that can be done with dataflow based analysis but looks like a difficult problem. For example is it possible to handle this case (X does not change)?

int Ret;
if (X)
  Ret = fgetc(fd);
else
  Ret = strtol(SomeString);
...
bool IsError;
if (X)
  IsError = (Ret == EOF);
else
  IsError = (Ret < -100 || Ret > 100);

The other "purpose" is to find one path with missing error check, the current code should do something like that. The path sensitivity is used here to store for a symbol from which function call it comes from, and probably to determine value of other symbols in the program if they appear in a comparison.

balazske updated this revision to Diff 279248.Jul 20 2020, 7:50 AM

NFC code improvements.
Detect expressions in conditions.

Some results with an improved version of this checker:
tmux: 6 results
duckdb: 23 results (1-2 false positive)
vim: 1 result
emacs: 25 results (more false positives mostly because ? operator that is easy to fix)

Would it be possible to publish these results on a public CodeChecker server?

balazske added a comment.EditedAug 6 2020, 3:31 AM

Test results for tmux are available here. (Could not manage to get other results uploaded.)

This test was done with an improved version of the checker that detects functions with possible NULL return value. (These functions are listed in the CERT ERR33-C rule, even if it is "impossible" that these fail the rule says that all of these should be checked.) Note tags here are experimental and not finished.

Test results for tmux are available here. (Could not manage to get other results uploaded.)

I guess this is a somewhat smarter version, correct? You seem to have NoteTags and handling for localtime etc. Anyways, I trust that the logic of mostly the same.

I don't know how likely it is for time management functions to fail, I guess its a rarity, but I guess this is exactly what static analysis is for. I think these results are nice, but as cheap of a response this is, I crave some more. You specifically mentioned a few false positives, I wonder what is up with them.

This checker can have two purposes, one is to verify that all paths have the error check, other is to find at least one path without error check. The first is the one that can be done with dataflow based analysis but looks like a difficult problem. For example is it possible to handle this case (X does not change)?

int Ret;
if (X)
  Ret = fgetc(fd);
else
  Ret = strtol(SomeString);
...
bool IsError;
if (X)
  IsError = (Ret == EOF);
else
  IsError = (Ret < -100 || Ret > 100);

The other "purpose" is to find one path with missing error check, the current code should do something like that. The path sensitivity is used here to store for a symbol from which function call it comes from, and probably to determine value of other symbols in the program if they appear in a comparison.

You are right, dataflow definitely has shortcomings that pathsensitivity doesn't, but it is true the other way around as well, which is why I believe (D72705#2141439) the combination of the two would be the correct solution, even if the question is whether the return value is checked on a given path of execution (that is precisely what the example in the comment demonstrates, not to mention that it could be far more obfuscated).


I think we have dedicated a lot of time to discuss our long term plans. I feel confident in my stance regarding the future of this checker, you seem to have a good grasp on it, we've gotten feedback a wide range of reviewers, and we also have this as a feature request outside Ericsson. Since this patch intends to lend an alpha checker, I think its about time we start talking about moving forward.

Here is what I like about this patch and want to see it in clang sooner rather than later:

  • Documentation is great. Its hard to overstate the value of that, when I literally have to sink in weeks into a codebase to understand whats going on due to the lack of it.
  • @NoQ mentioned the callback choice may not be ideal/wrong, but I personally disagree. Its far easier, and I think cheaper to climb up on the AST. The high level idea behind the visitor analyzing whether a statement constitutes as a check is great, I don't we have anything similar.
  • I think you tested the added functionality for an alpha checker quite well.
  • If my worries (did we think about all the cases that could constitute as a check?) won't be an issue, the infrastructure proposed by this patch seems easily expandable.

Here is what I want to see moving forward:

  • This comment D72705#2088319 worries me. @NoQ, could you please expand on this? I feel like you have a legitimate worry I'm not seeing, and I perceive you to be a bit skeptical about this whole shebang. While I'd hate to stall the progress of this patch, it'd be nice to be settled on this before moving too fast forward.
  • More evaluations. I realize that reviewers don't demand too much on this front for an initial patch for an alpha checker, but both the problem and our different visions on the solution could use some backing up. It would be a shame if an issue we could've easily foreseen came about that would require serious architectural changes. What was the problem in uploading the results you already had on hand?

I left some inlines, but I didn't go too nitty just yet. For the time being, lets have these two goals met, and after that I'll definitely do my best to help you get this landed ASAP.

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
56–61

So, for the time being we're saying that a checking must involve a binary operator. That sounds about right, but I wonder if there is a case we're not thinking about.

64

This is the value we're checking against, I think the name KnownValue doesn't convey this.

80–81

So if we failed to get retrieve the value we're checking against, we automatically assume that its not a proper check? Shouldn't we be conservative here? What if that value is something like getEOFValue()?

84

We have a smarter EOF getter thanks to @martong, don't we?

285–288

This is a very broad statement. I can't come up with a counterexample on top of my head, but we should keep this in mind. What this totally deserves however, is a distinct warning message, like "xth argument to system/standard function call is an unchecked return value".

Please put a TODO here.

296–297

Aha, interesting. So the thing to note here is that if the parent (which is stripped of all the cast/paren expressions) in a non-call expression, it constitutes as an improper use before checking. Again, I don't have an immediate counter example, but let's keep this in mind as a likely source of false positives. Which may not be too bad, we'll at least immediately be notified that there are additional cases to handle.

330–331

Why?

429–433

Ah, okay, so you mean to check whether someone did something like this:

// Silence compiler warning.
(void)localtime(...);

I don't think that the parent though will get you the correct answer, how about this:

// Silence compiler warning.
(void) (coin() ? NULL : localtime(...));

Please put a TODO here.

balazske marked an inline comment as done.Aug 7 2020, 9:01 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
56–61

For the "fputs" case yes, comparison to EOF (or negative) can be detected by binary operator. The extended version has support for unary operator too (and some other tricks) (detect check of nullness of a value).

80–81

This case needs attention, now it is detected as failure (maybe the function can return Optional<bool> to indicate a non-determinable case).

330–331

This comment looks invalid now, can be removed. (VisitIfStmt already handles the case.)

429–433

Probably this case can be handled by the ErrorCheckTestStmtVisitor in better way.

balazske updated this revision to Diff 284375.Aug 10 2020, 8:03 AM
balazske marked an inline comment as done.
  • Renames.
  • Changed behavior if function call is compared to returned value.
balazske marked 3 inline comments as done.Aug 10 2020, 8:22 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
64

Renamed to ValueToTestAgainst.

80–81

This case is now not error.
A test is added for this case.

int R = fputs("str", file());
if (R == getEOFValue())
  return;

This is no warning now (but getEOFValue can return anything).

84

tryExpandAsInteger is used now.

221–224

CalledFunctionData was renamed to FunctionCallData and CFD variables to Call. It looks better now.

balazske updated this revision to Diff 284741.Aug 11 2020, 8:18 AM
balazske marked 2 inline comments as done.
  • Rebase
  • Changed testBinOpForCheckStatement

More results in CodeChecker: emacs_errorreturn

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
80–81

The whole function is now refactored to have better behavior.

balazske updated this revision to Diff 285057.Aug 12 2020, 6:05 AM

Improved comments, code clean-up.
Improved detection of explicit cast to void.

More results in CodeChecker: emacs_errorreturn

These results (and the ones in tmux) seems to be quite convincing, even though you have found some false positives. Also, I second @Szelethus 's opinion that this is a valuable checker and we should welcome this in the pack as an alpha checker :)

Test results for tmux are available here. (Could not manage to get other results uploaded.)

This test was done with an improved version of the checker that detects functions with possible NULL return value. (These functions are listed in the CERT ERR33-C rule, even if it is "impossible" that these fail the rule says that all of these should be checked.) Note tags here are experimental and not finished.

Seems like the findings are mostly related to time management functions. So, it would be interesting to see how the results correlate/divergate to the StdLibraryFunctionChecker's POSIX time handling patch (this is a task for me to do the measurements).

More results in CodeChecker: emacs_errorreturn

Something's not quite right, e.g. at https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=emacs_errorreturn&review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&report-id=7258 there's a bug path element Function '???' called [...]. This seems to be the case for almost every report... but I found no indication where this "???" could come from, in the patch...

More results in CodeChecker: emacs_errorreturn

Something's not quite right, e.g. at https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=emacs_errorreturn&review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&report-id=7258 there's a bug path element Function '???' called [...]. This seems to be the case for almost every report... but I found no indication where this "???" could come from, in the patch...

Because that is an improved version of the checker. The current form is not testable because it detects too less functions. To have more results functions were added that may return NULL on error and a missing comparison to NULL is detected (similar as missing comparison to EOF in this patch). (To have the check for NULL some other improvements were made.) The '???' is part of development of note tags, the name of the called function is to be placed there (probably there is better way of doing these reports).

Szelethus requested changes to this revision.EditedAug 26 2020, 4:10 AM
Szelethus added a subscriber: bruntib.

I debated this report with @bruntib, depending from where you look at it, its either a false positive or not -- in short, this is what happens:

char *p = /* some assumed to be valid c string */
end = strchr(p, '\n');
if (end - p > 4) { // <-- warning here on end's invalid use
}

the guarded code wouldn't be executed, should end be zero. Now, if we interpret the rule in a very literal sense, that

The successful completion or failure of each of the standard library functions listed in the following table shall be determined either by comparing the function’s return value with the value listed in the column labeled “Error Return” or by calling one of the library functions mentioned in the footnotes.

then indeed, it doesn't compare the return value with the item in the table, but it does compare it with superset of it. Now, we can either say that

  1. A superset isn't good enough, because its a sign of code smell, or
  2. Judge that the user probably did handle the error case, albeit in an unconventional way, and report like these don't provide any value.

I don't think we can realistically pursue point (2.). We would need to widen the context (by going further up the AST with Stmt::getParent()) we're considering for a potential check. However, forming a non-checking expression that we don't immediately report can quickly get out of control:

char *p = /* some assumed to be valid c string */;
end = strchr(p, '\n');
int result = end - p;
if (result > 4) {
}

You would be right to believe, without context from the code that is yet to be executed, that the computation of result is invalid, because end is unchecked. However, the code complies with the rule interpreted by (2.). This seems to be a problem that can have infinite complexity without very severe restriction on the analysis. Indeed, if we wander too far from the original function call (by creating values that descend from the return value to be checked), being sure that no checks happened (by point 2.) quickly becomes unmanageable.

So, for all practical purposes, we're locked to option (1.). However, the warning message this way is imprecise -- we're not enforcing whether the error value was checked for error, we're enforcing whether the return value was checked against its respective failure value, and the message should reflect that. In fact, the checker description, and some comments should be clarified in this regard as well.

I would imagine the pool of functions that deserve such incredibly strict enforcement is rather small. Time management, maybe, but certainly not string modeling functions, because problems related to them are solved by a completely different checker infrastructure (CStringChecker), just as stream modeling functions and memory management. Maybe we should only look for statement-local bugs (basically a discarded return value) for functions that don't demand this rigidness, but I suspect there is a clang-tidy check already out there doing it. Seems like I got what @NoQ meant here.

In D72705#2088319, @NoQ wrote:

I still don't understand how do we enable this checker, i.e. for whom and how often. Is there anything we'll be able to turn on by default, like maybe warn on all functions that wear __attribute__((warn_unused_result))?


I'm very happy we got another round of evaluations, we've gotten some invaluable information out of it. For future patches, it would be great to have your input on at least some of the reports as you're publishing them. For the latest project, I think there is clear intent from the developer to avoid null pointer misuses -- you should justify why a report is still appropriate there.

You mentioned in D72705#2187556 that on duckdb you stumbled across 1-2 false positives, but also that you found 23 reports, and that you ran the checker on vim. This may be okay for an alpha checker, or maybe not, if it points to a fundamental problem in your approach. Please publish them before updating the patch. If all goes well and those cases could be patched up later, here are the next important steps:

  • Have a warning message that precisely states what, and how the rule was violated, even if its done with placeholders, even if you only wanna place a FIXME and address this later. Either is fine. By the time we ship this checker, a message that delivers the following information, even if phrased differently, would be great: "According to the CERT rule ERR33-C, the <return value/output parameter> must be explicitly checked whether its <equal/greater/less than> <error value>."
  • Expand on the checker description similarly. Mention that checking against the superset of the error value isn't sufficient.
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
513

I'm fine with the checker being here, but the nature of the problem makes it definitely a good candidate to place it in an optin package. However, recent discussion about the changing of the package system, and that we're still in alpha, this isn't an issue to address immediate.

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
189–190

This report made me imagine a case where the value we're checking against is the appropriate error value, but is only representable by an SVal. We could say that according to the rule the error value should be hardcoded, so this might just be okay, but I'd keep an eye on it (maybe with a NOTE:?).

197–202

We talked about this during a meeting, but this is interesting. Seems to be directly related to the highlighted code here -- we were not able to prove that the value we're testing against is the appropriate error value, so we guess (except if its a function call) that it isn't. Normally, I'd say that if we didn't manage to prove that the other value is incorrect, we should conservatively suppress the report, but according to point (1.) I just talked about, we could say that the rule enforces a hardcoded NULL.

At the end of the day, your evaluations on strchr are great for discussion purposes to see whether your infrastructure works, but reports on it might be too verbose and not valuable enough to ship into the real world.

398

I don't think this adds any value, and in fact makes reading the bug report harder, because the range is outside of the actual error location. This should be replaced (and not accompanied) with a NoteTag.

This revision now requires changes to proceed.Aug 26 2020, 4:10 AM

The summary of this last discussion is that it is not acceptable to have only the simple check for the explicit comparison with a fixed constant. At least not for return types where the "implicit" check (a check that is always true or false for the error return value) is possible, for example the char* case. For functions that return a sort of "handle" (mainly a pointer to a struct that is not normally used with pointer arithmetic) the checker can still be useful.

Another solution for the problem is if the system calls are modeled in a way that there is always a state split between error end non-error (we will have a path where it is known that the specific variable can be only (for example) NULL and this can be detected by other checkers).

Another solution for the problem is if the system calls are modeled in a way that there is always a state split between error end non-error (we will have a path where it is known that the specific variable can be only (for example) NULL and this can be detected by other checkers).

I think this approach could be easily modeled in StdLibraryFunctionCheckers with the form of Cases (branches). E.g. https://github.com/llvm/llvm-project/blob/f20e6c7253859454c2f39adae19d80a31a0456a9/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp#L1139

Also, Comments about the branches:
https://github.com/llvm/llvm-project/blob/f20e6c7253859454c2f39adae19d80a31a0456a9/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp#L13

I debated this report with @bruntib, depending from where you look at it, its either a false positive or not -- in short, this is what happens:

char *p = /* some assumed to be valid c string */
end = strchr(p, '\n');
if (end - p > 4) { // <-- warning here on end's invalid use
}

the guarded code wouldn't be executed, should end be zero. Now, if we interpret the rule in a very literal sense, that

The successful completion or failure of each of the standard library functions listed in the following table shall be determined either by comparing the function’s return value with the value listed in the column labeled “Error Return” or by calling one of the library functions mentioned in the footnotes.

then indeed, it doesn't compare the return value with the item in the table, but it does compare it with superset of it. Now, we can either say that

  1. A superset isn't good enough, because its a sign of code smell, or
  2. Judge that the user probably did handle the error case, albeit in an unconventional way, and report like these don't provide any value.

I don't think we can realistically pursue point (2.). We would need to widen the context (by going further up the AST with Stmt::getParent()) we're considering for a potential check. However, forming a non-checking expression that we don't immediately report can quickly get out of control:

char *p = /* some assumed to be valid c string */;
end = strchr(p, '\n');
int result = end - p;
if (result > 4) {
}

You would be right to believe, without context from the code that is yet to be executed, that the computation of result is invalid, because end is unchecked. However, the code complies with the rule interpreted by (2.). This seems to be a problem that can have infinite complexity without very severe restriction on the analysis. Indeed, if we wander too far from the original function call (by creating values that descend from the return value to be checked), being sure that no checks happened (by point 2.) quickly becomes unmanageable.

So, for all practical purposes, we're locked to option (1.). However, the warning message this way is imprecise -- we're not enforcing whether the error value was checked for error, we're enforcing whether the return value was checked against its respective failure value, and the message should reflect that. In fact, the checker description, and some comments should be clarified in this regard as well.

I would imagine the pool of functions that deserve such incredibly strict enforcement is rather small. Time management, maybe, but certainly not string modeling functions, because problems related to them are solved by a completely different checker infrastructure (CStringChecker), just as stream modeling functions and memory management. Maybe we should only look for statement-local bugs (basically a discarded return value) for functions that don't demand this rigidness, but I suspect there is a clang-tidy check already out there doing it. Seems like I got what @NoQ meant here.

In D72705#2088319, @NoQ wrote:

I still don't understand how do we enable this checker, i.e. for whom and how often. Is there anything we'll be able to turn on by default, like maybe warn on all functions that wear __attribute__((warn_unused_result))?


I'm very happy we got another round of evaluations, we've gotten some invaluable information out of it. For future patches, it would be great to have your input on at least some of the reports as you're publishing them. For the latest project, I think there is clear intent from the developer to avoid null pointer misuses -- you should justify why a report is still appropriate there.

You mentioned in D72705#2187556 that on duckdb you stumbled across 1-2 false positives, but also that you found 23 reports, and that you ran the checker on vim. This may be okay for an alpha checker, or maybe not, if it points to a fundamental problem in your approach. Please publish them before updating the patch. If all goes well and those cases could be patched up later, here are the next important steps:

  • Have a warning message that precisely states what, and how the rule was violated, even if its done with placeholders, even if you only wanna place a FIXME and address this later. Either is fine. By the time we ship this checker, a message that delivers the following information, even if phrased differently, would be great: "According to the CERT rule ERR33-C, the <return value/output parameter> must be explicitly checked whether its <equal/greater/less than> <error value>."
  • Expand on the checker description similarly. Mention that checking against the superset of the error value isn't sufficient.

Took one beer to read through this :D
Besides the joke, this is a really insightful comment and thanks for sharing your thoughts. I absolutely agree that the checker should state in the warning messages that the value was not checked against its respective failure-value. Also thank you Balazs for your hard work!

The summary of this last discussion is that it is not acceptable to have only the simple check for the explicit comparison with a fixed constant. At least not for return types where the "implicit" check (a check that is always true or false for the error return value) is possible, for example the char* case.

Indeed, though I think it is okay to keep them in the checker while this is being developed, because it still tests the infrastructure well enough. We should just remember to fine tune the set of functions we're checking before moving out of alpha.

For functions that return a sort of "handle" (mainly a pointer to a struct that is not normally used with pointer arithmetic) the checker can still be useful.

Agreed. This is true for some out-parameters as well.

Another solution for the problem is if the system calls are modeled in a way that there is always a state split between error end non-error (we will have a path where it is known that the specific variable can be only (for example) NULL and this can be detected by other checkers).

@NoQ had a great comment about the dangers of this: D79432#inline-757474. If we still end up wanting to do this, then I agree with @martong, they should be placed in StdLibraryFunctionsChecker.

I am not sure if this checker is worth to further development. A part of the found bugs can be detected with other checkers too, specially if the preconditions of many standard functions are checked (more than what is done now) and by modeling the function calls with assumed error return value. Some return codes does not cause invalid preconditions but then it is often enough to have any check for the returned value, or the problem can be found by the taint checker (this can be improved).

A improved version of the checker is added for further discussion (or should continue here?) and to show the improved checker code.
See D88312.

Szelethus added a comment.EditedTue, Sep 29, 9:59 AM

I am not sure if this checker is worth to further development. A part of the found bugs can be detected with other checkers too, specially if the preconditions of many standard functions are checked (more than what is done now) and by modeling the function calls with assumed error return value.

We shouldn't throw all the knowledge away, once its in our hands. What you're suggesting as an alternative is using state splits, something that is obviously expensive, and we can't know whether that state split is always just / constructive.


Since we had private meetings about this, it would be great to share the overall vision going forward. While we shouldn't burden @NoQ with having to review this checker line-by-line, he might have valuable input on what we're going for.

I'd like to ask you to write a mail on cfe-dev, and bring forward a formal proposal:

  • Explain ERR33-C, and your interpretation of it. Highlight how you want to see very explicit error checking against the concrete return value in some cases, and not in others. Provide a list of functions where you think, and why you think that vigorous checking is appropriate, and a list for those where a more lenient one could be used.
  • Explain why you picked symbolic execution to solve this problem.
  • Explain how this problem could be solved, and the pros/cons of them:
    • Making state splits in StdLibraryFunctionChecker
    • Expanding on taint analysis
    • Check constraints on a return value in a new checker (this is impossible, sure, but why?)
    • GDM tracking of values to be checked in a new checker
  • You picked no4 -- justify it.

Since you made considerable progress already, it would be great to have talk about how you are implementing this, and why. For instance, "I recognize appropriate function in checkPostCall, and mark their return value as unchecked in the GDM. Later, in checkLocation, if I find that such a value is read from, I'll analyze the surrounding code to find out whether its a rule-compliant comparison of it, if it is not, I emit a warning. This is done by ascending the AST with a ParentMap, and running a statement visitor on the parent statement". Code examples are always amazing, demonstrate on a piece of code how if(someReturnValue != 0), or something similar would be analyzed.


I hope I didn't sounds condescending or demanding, it was definitely not my intent. Its a joy to see your works come to life, there are a lot of smarts to marvel in! With that said, the reviewing process has shown some significant problems.

D71510 was submitted 10 months ago -- you obviously didn't work on it non-stop, but the fact that some pivotal aspects of your solution was realized by me only a month ago despite doing my best to follow the development is very unfortunate indeed. I'm not sure anyone else realized the extent of it either. Changes on StreamChecker followed a very similar trend, I felt like I had to understand, explain, and prove the correctness of the change for myself. As I made progress on it, sometimes the patches were split up, reuploaded and abandoned, like a multi-headed hydra. We ought to draw some lessons from this.

As an author, you can make the reviewers job a lot easier by:

  • If the change is big enough, have a round on the mailing list before writing too much code. Scouting ahead is fine, but its not worth going too far.
  • Provide a thorough summary with examples, drawings (D70411#1754356), whatever that helps to convey the message. If the code you're changing took considerable time to understand, it will probably take considerable time for reviewers as well. D86874, D55566, D86135 and D86736 are great examples. D63954 keeps it short, but provides a link to more discussions.
  • For a new checker, upload a very slimmed down version of your prototype. Its okay if it has FP/FN problems, or crashes on some weird input, its alpha for a reason. Its rare that for demonstration purposes you really need 500+ LOC. This allows everyone to discuss followup steps as well (though I don't think you ever left me in the dark regarding that).
  • If a high level objection/question rose, its not worth adding new features, or even updating it much, unless it directly clarifies some trivial misunderstanding.
  • When you publish results, give your input on them. Are you satisfied with the findings? Are there notable false positives? Could they be fixed easily?
  • Laugh at how I used to do none of these, again, because its funny (D45532#1083670).

There are a lot of lessons to be taken on my side as well. I should've been able to realize this feature earlier, looking at the code now, its wildly obvious. I also could've asked for more info earlier and with more persistence. I don't pair my objections with enough appreciation for the invested effort.

Lets try to get this right, or done better, on the next patch!