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

[balazske]

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

[balazske]

I am experimenting with somewhat other check algorithm for this problem, the review of this code can be suspended for now.

[balazske]

A better (?) implementation.

[baloghadamsoftware]

Hello,

This checker is an alternative approach to our not yet published statistics-based checkers in a way: 2019 EuroLLVM Developers’ Meeting: A. Balogh “Statistics Based Checkers in the Clang Static Analyzer”

There are two main differences: The first is that in the statistics-based Special Return Value checker we read the function names from a YAML file. There is no other way because the data was previously dynamically collected, but we can also add a static part to it which contains the functions hard-wired in your checker. I think that is a better approach.

The other, even more important difference is that in the Special Return Value checker we do not check explicitly whether the return value was compared to NULL pointer but we fork a new execution path where it is a NULL pointer and expect other checkers to report a bug on this path. I wonder which approach is the better one: Your approach also finds cases which our checkers can not, e.g. there is not enough memory for malloc(). However it also finds more false positives where the return value was left intentionally unchecked because in that particular case no error can happen.

[balazske]

Small bugfixes, added some tests.

[balazske]

Added garbage collection and better problem detection.

Without preventing garbage collection it is not possible to improve the checker
(in this way).

[balazske]

Using check::DeadSymbols.

[balazske]

I am still unsure about how this checker works if the function to check is "modeled" or evaluated by another checker. Then the function may have already a constrained value at the PostCall event (for example if a malloc fails) and the basic idea of this checker that the constraints on the return value indicate the checks that the program makes does not work. So it may be better to check for the initial constraint somehow and if found ignore that function. Or create a new state where the function call's value is not constrained.

[baloghadamsoftware]

Please consider the following test cases:

void test_NullCorrectCheck3() {
  void *P = aligned_alloc(4, 8);
  use(*P); // A developer inserted this line before the check by mistake. This will be a null pointer dereference.
  if (P == NULL) {
  }
}

Or:

void test_NullCorrectCheck3() {
  void *P = aligned_alloc(4, 8);
  if (dice()) { // A deloper introduced a new branch, but by mistake, before the check.
    if (P == NULL) {
      use(*P);
    }
  } else {
   use(*P); // No check in this branch, thus a null pointer dereference.
  }
}

False positive (or not?):

void g(void*); // Unknown function

void test_NullBadCheck1() {
  void *P = aligned_alloc(4, 8);
  g(P); // If g() checks its parameter for null, then false positive. If not, then true positive.
}

[balazske]

I wanted to use the existing infrastructure (ExprEngine) to test what conditions are applied to the function return value. I did not found other way to get this information. So the checker observes what checks are applied to the return value by making assumptions on it. The checker is only designed to detect if specific checks are made on the return value, not what other problems the return value (NULL or -1 or other) can cause, this is handled by other checkers. 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 the first case the checker can be improved probably by detecting that the value usage is before the error check program point. But this is hardly possible to do because the error check code may be split into multiple branches (or switch if it is a numerical value) so the check happens not at a single point and we can not be sure if a value access is part of the error check or not.

The second case is not detectable with this kind of checker, unless it is possible to find the values that a conditional branch depends on (to find out that the "dice" call does not depend on the return value of aligned_alloc).

The third case can be improved by checking for "escape" of the value to check. Otherwise this should give a false positive now.

Probably it is better to make other checkers for special purposes (because the checker in this version is some "abuse" of the path sensitive analysis and the above mentioned problem). If for many of the listed functions there is checker that verifies validity of the input parameters, or tracks if a special error value was returned from the function or the objects (for example file descriptors) are used without error check. Still some kind of errors are not detectable, for example the fread or strtol cases.

[balazske]

This is the original version of the (complete) checker, but now outdated:
https://reviews.llvm.org/D71510

[balazske]

Adding a new implementation.
Now the conditions itself are checked, not the constraints.
The appearance of the value to check is detected and tested if
it is inside an error check-looking construct.
The location of the error check is available and use before check is
detected too.
More tricky cases of error check code are not found but
these should be rare or "questionable" cases.

Probably some possibilities are missed from this code.
There is still some debug code left.

[baloghadamsoftware]

Please add tests for all the functions in the list.

[balazske]

• Bug fixes in the generic code.
• Added new check types.

[balazske]

In D72705#1854641, @baloghadamsoftware wrote:

Please add tests for all the functions in the list.

How to handle the combinatoric explosion in test code?
It is too much code to add all functions to system-header-simulator.h and then all functions with all cases to the test files.
Even for now, all cases with all kinds of check types can result in too big test code.
This is possible only with automatic code generation.

[Szelethus]

Apologies for inserting myself in here :)

Please use the "[analyzer]" tag instead of "[clang][checkers]" in future changes, because we've used that traditionally for years, and most of us are automatically subscribed to it. The term "checker" is mostly known for these modules in the analyzer, but I'm pretty sure its used in many other places, considering that the main selling point of clang is "checking" the source code better then most other compilers.

---

You stated that

First simple implementation, infrastructure is ready.

Yet the checker code is still about as long as in D71510. I share the sentiment of @NoQ in D71510#1818438, this is far too big to review thoroughly, even if we're putting it in alpha first and leave the gritty bits for later. Despite the patch's aim is to introduce an infrastructure, I'm reading code about function filters, special cases for variadic functions, explicit casts to void, 3 different classes for 3 different kinds of return values, sometimes we "save" arguments, but not always, and many other things I failed to understand.

I like to pinpoint to this comment that explains well why we historically preferred smaller patches in the analyzer: D45532#1083670 (which is on, ironically, my patch).

I see that you're using a statement visitor, which always raises the question as to why wasn't a matcher sufficient. Although, I'm not exactly sure why we're using a syntactic check for a problem so inherently pathsensitive at all, and I couldn't really find an explanation in the code.

---

I'm not sure whether we want to hunt cases like this down, or at least not with this checker:

void test_Null_UnusedVar() {
  void *P = aligned_alloc(4, 8);
} // expected-warning{{Return value was not checked}}

Isn't this a dead store or unused variable issue? Or, in this specific case, also a leak.

---

There were valid questions raised by D71510#1818438, and very few of those were addressed. I tried looking up discussions on discord, cfe-dev, internal mailing lists, the previous patch and I didn't find much. I would prefer to first agree on what we're shooting for and how, and have a battle plan drawn and explained in the code, such as

1. We're sifting out the functions whose return value should always be checked.
2. Should the function be of interest, we're adding its return value to the GDM.
3. Do some analysis on whether the return value was validated.
   1. For this, we're using syntactic analysis (?), which starts with ...
   2. If ... then ...
   3. etc
4. Report errors if so.

The test cases convince me that there is real value to your work, but I'd urge you to take a few steps back and leave room for discussion, and reduce the scope of this patch to only establish the infrastructure, and maybe find the simplest case. I can't stress enough that the idea is great, and even if this ends up as an optin checker we should have it, so please don't take my stance as anything else but caution :)

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp

45

Please avoid calling any class a checker if they are not inheriting from Checker<>.

[Szelethus]

In D72705#1838324, @balazske wrote:

I am still unsure about how this checker works if the function to check is "modeled" or evaluated by another checker. Then the function may have already a constrained value at the PostCall event (for example if a malloc fails) and the basic idea of this checker that the constraints on the return value indicate the checks that the program makes does not work. So it may be better to check for the initial constraint somehow and if found ignore that function. Or create a new state where the function call's value is not constrained.

Under evaluation, you mean evalCall()? Could you please detail what your worry is, and what the essence of the current solution is?

[balazske]

In D72705#1859157, @Szelethus wrote:

In D72705#1838324, @balazske wrote:

I am still unsure about how this checker works if the function to check is "modeled" or evaluated by another checker. Then the function may have already a constrained value at the PostCall event (for example if a malloc fails) and the basic idea of this checker that the constraints on the return value indicate the checks that the program makes does not work. So it may be better to check for the initial constraint somehow and if found ignore that function. Or create a new state where the function call's value is not constrained.

Under evaluation, you mean evalCall()? Could you please detail what your worry is, and what the essence of the current solution is?

This comment applies to a previous solution for the problem. In that case the checker worked by testing the constraints that are put on the "return value". The "return value" is the SymbolRef that comes from the function call return value. The main idea was that initially this value is unconstrained, then the conditions in the evaluated code generate constraints for it. By checking these constraints it is possible to reason about the conditions that were applied in the code to the value. And find out if a specific condition was applied, the one that means that the value was checked for error code.

int X = f();
// X is not constrained
if (X) {
  // X is constrained to non-zero
} else {
  // X is constrained to zero
}

The constraints accumulate during an execution path and at a time the required state is reached (or not). In this example code we know at the end that X was checked for "nullness" (it can be either only zero or anything except zero). This was the first idea but does not work always correct and requires too exact error checking in the evaluated code. In a next solution the searched constraint was only that the return value should be constrained to outside of the error return range. Basically a single execution path with this condition was searched. (In the simple code above, search for one execution path where X is for sure not zero at the end.) This solution has problems too. So the whole way of this constraint-based search was abandoned.

The comment is about if initially the symbol for called function (f in the code above) is constrained. 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.

[balazske]

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?

[balazske]

Uploading new diff with added comments and renaming.
(And a new feature of leaking pointer detection.)
The code is at least saved here, can be used as starting point for smaller changes.

[Szelethus]

In D72705#1861750, @balazske wrote:

Uploading new diff with added comments and renaming.

Great, very much appreciated! It took a loong time, but I think I got whats going on. So, step by step, this is what we're doing if a function of interest is called:

1. Put the return value in a map because we need to keep an eye out for it.
2. If that value is ever read (which we'll know about in checkLocation), we will thoroughly check that statement whether it
   1. Qualifies as a check for an error, in which case we take the return value out of the map
   2. Is a read, in which case we emit a warning
   3. Is an assignment (like for (P = aligned_alloc(4, 8); P;) or void *P = aligned_alloc(4, 8);), in which case we jump to step 2 with the current statement's parent in the AST.
   4. Does nothing, in which case we leave it in the map for now.

That is the essence of what's going on, is that correct?

The code is at least saved here, can be used as starting point for smaller changes.

Thanks! For now, lets discuss how the current solution works, let other reviewers take a look at the high level idea, and after reaching some consensus, try to see how we can split this patch up into logical chunks.

In D72705#1859711, @balazske wrote:

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.

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?

My point was different. Are we sure that it is an error not to check the error value here, before going out of scope?:

void test_Null_UnusedVar() {
  void *P = aligned_alloc(4, 8);
} // expected-warning{{Return value was not checked}}

I'm not debating whether this core is incorrect, only the fact that not checking the return value isn't the issue here. In any case, let's drop this topic for now, because it adds very little to the real point of this patch, seeing how the problem should be solved as a whole. We should revisit this specific case in a small, followup patch.

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp

221–224

The comments leave me to believe that the correct class name would be FunctionCallData, right? Because FnInfo does the job of describing the called function, and this one deals with the actual call.

[balazske]

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

[Szelethus]

In D72705#1863499, @balazske wrote:

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]

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]

Improved tests.

[Szelethus]

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.

[Szelethus]

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

[balazske]

Ping !

[balazske]

ping

[martong]

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?)

[balazske]

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).

[balazske]

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.

[martong]

In D72705#2036066, @balazske wrote:

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]

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

[martong]

In D72705#2041779, @balazske wrote:

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.

[balazske]

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]

In D72705#1871996, @Szelethus wrote:

@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]

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

[Szelethus]

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.

[balazske]

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.

[balazske]

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".

[Szelethus]

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.

In D72705#2086994, @balazske wrote:

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.

In D72705#1863381, @Szelethus wrote:

In D72705#1859711, @balazske wrote:

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.

[balazske]

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.

[Szelethus]

In D72705#2143405, @balazske wrote:

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)]

[balazske]

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
}

[Szelethus]

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.

[balazske]

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]

NFC code improvements.
Detect expressions in conditions.

[balazske]

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)

[Szelethus]

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

[balazske]

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.

[Szelethus]

In D72705#2199333, @balazske wrote:

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.

In D72705#2161576, @balazske wrote:

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]

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]

• Renames.
• Changed behavior if function call is compared to returned value.

[balazske]

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]

• Rebase
• Changed testBinOpForCheckStatement

[balazske]

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]

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

[martong]

In D72705#2210255, @balazske wrote:

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 :)

[martong]

In D72705#2199333, @balazske wrote:

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).

[whisperity]

In D72705#2210255, @balazske wrote:

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...

[balazske]

In D72705#2213300, @whisperity wrote:

In D72705#2210255, @balazske wrote:

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]

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.

[balazske]

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).

[martong]

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

[martong]

In D72705#2238487, @Szelethus wrote:

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!

[Szelethus]

In D72705#2241542, @balazske wrote:

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.

[balazske]

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).

[balazske]

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]

In D72705#2274568, @balazske wrote:

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!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def MismatchedDeallocatorChecker : Checker<"MismatchedDeallocator">,
   Documentation<HasDocumentation>;
 
 def VforkChecker : Checker<"Vfork">,
   HelpText<"Check for proper usage of vfork">,
   Documentation<HasDocumentation>;
 
 } // end "unix"
 
 let ParentPackage = UnixAlpha in {
 
 def ChrootChecker : Checker<"Chroot">,
   HelpText<"Check improper use of chroot">,
   Documentation<HasAlphaDocumentation>;
 
+def ErrorReturnChecker : Checker<"ErrorReturn">,
+  HelpText<"Check for unchecked error return values">,
+  Documentation<HasAlphaDocumentation>;
+
 def PthreadLockChecker : Checker<"PthreadLock">,
   HelpText<"Simple lock -> unlock checker">,
   Dependencies<[PthreadLockBase]>,
   Documentation<HasAlphaDocumentation>;
 
 def StreamChecker : Checker<"Stream">,
   HelpText<"Check stream handling functions">,
   Documentation<HasAlphaDocumentation>;

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   DebugIteratorModeling.cpp
   DeleteWithNonVirtualDtorChecker.cpp
   DereferenceChecker.cpp
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
   EnumCastOutOfRangeChecker.cpp
+  ErrorReturnChecker.cpp
   ExprInspectionChecker.cpp
   FixedAddressChecker.cpp
   FuchsiaHandleChecker.cpp
   GCDAntipatternChecker.cpp
   GenericTaintChecker.cpp
   GTestChecker.cpp
   IdenticalExprChecker.cpp
   InnerPointerChecker.cpp

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp

+//===-- ErrorReturnChecker.cpp ------------------------------------*- 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 file defines ErrorReturnChecker, a builtin checker that checks for
+// error checking of certain C API function return values.
+// This check is taken from SEI CERT ERR33-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors
+//
+// About the checker:
+// It involves a predefined set of system call functions that can fail and
+// return a specific error code on failure. (This list is provided by the CERT
+// rule.) The checker tries to verify if there is a statement in the code that
+// checks the returned value. The way of how this check should be done is
+// function-specific. The first use (that is not assignment or pass to function)
+// of the return value should be the error check. Any other first use of the
+// returned value (except assignment or pass to function call) is reported as
+// warning. Additionally, if the return value is not used at all a warning is
+// generated for unchecked return value. The warning can be suppressed by
+// explicit cast to void.
+//
+// This checker is currently not usable for real code. More checked functions
+// and return value check types must be added.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/Expr.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include <functional>
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class ErrorReturnChecker;
+
+/// Describe how to check the return value (of a function) for error.
+class ErrorReturnCheckKind {
+public:
+  ErrorReturnCheckKind(const ErrorReturnChecker *Checker) : Checker{Checker} {};
+  virtual ~ErrorReturnCheckKind(){};
+
+  /// Test if an encountered binary operator where the return value is involved
+  /// is a valid check statement. The return value appears in one side of the
+  /// operator, the other side is passed in `OtherSideExpr`.
+  /// Return false if warning should be reported by the checker, otherwise true.
+  /// Return false only if it is sure that problematic code was found.
+  virtual bool testBinOpForCheckStatement(CheckerContext &C, QualType RetTy,
+                                          const BinaryOperator *BinOp,
+                                          const Expr *OtherSideExpr,
+                                          bool OtherSideIsRHS) const = 0;
+
+protected:
+  const ErrorReturnChecker *const Checker;
+};
+
+/// Check for comparison to EOF or less than zero.
+/// Accept a binary operator if the return value is compared to EOF or the
+/// comparison ensures a (any) negative value. This check is used for system
+/// functions where the error return value is "EOF or negative".
+class EOFOrNegativeErrorReturn : public ErrorReturnCheckKind {
+public:
+  EOFOrNegativeErrorReturn(const ErrorReturnChecker *Checker)
+      : ErrorReturnCheckKind{Checker} {}
+
+  bool testBinOpForCheckStatement(CheckerContext &C, QualType RetTy,
+                                  const BinaryOperator *BinOp,
+                                  const Expr *OtherSideExpr,
+                                  bool OtherSideIsRHS) const override;
+};
+
+/// Description of an API function to check.
+struct FnInfo {
+  /// Error return check kind for the function.
+  ErrorReturnCheckKind *ErrorReturnKind;
+
+  /// Return type of the function (initialized at runtime).
+  mutable QualType RetTy;
+
+  FnInfo(ErrorReturnCheckKind *ErrorReturnKind)
+      : ErrorReturnKind(ErrorReturnKind) {
+    ;
+  }
+};
+
+/// Information about a specific function call that has an error return code to
+/// check. This data is stored in a map and indexed by the SymbolRef that stands
+/// for the result of the function call.
+struct FunctionCallData {
+  /// Point out the kind of the function that was called.
+  const FnInfo *Info;
+  /// Source range of the calling statement.
+  SourceRange CallLocation;
+
+  FunctionCallData(const FunctionCallData &Call)
+      : Info(Call.Info), CallLocation(Call.CallLocation) {}
+  FunctionCallData(const FnInfo *Info, const SourceRange &CallLocation)
+      : Info{Info}, CallLocation{CallLocation} {}
+
+  FunctionCallData &operator=(const FunctionCallData &Call) {
+    Info = Call.Info;
+    CallLocation = Call.CallLocation;
+    return *this;
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddPointer(Info);
+    ID.AddInteger(CallLocation.getBegin().getRawEncoding());
+  }
+
+  bool operator==(const FunctionCallData &Call) const {
+    return Info == Call.Info && CallLocation == Call.CallLocation;
+  }
+};
+
+class ErrorReturnChecker
+    : public Checker<check::PostCall, check::Location, check::PointerEscape> {
+  mutable std::unique_ptr<BuiltinBug> BT_UncheckedUse;
+
+  void checkAccess(CheckerContext &C, ProgramStateRef State, const Stmt *LoadS,
+                   SymbolRef CallSym, const FunctionCallData *Call) const;
+  ProgramStateRef processEscapedParams(CheckerContext &C, const CallEvent &Call,
+                                       ProgramStateRef State) const;
+  const FnInfo *findFunctionToCheck(CheckerContext &C,
+                                    const CallEvent &Call) const;
+
+public:
+  void initialize(CheckerManager &Mgr);
+
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkLocation(SVal L, bool IsLoad, const Stmt *S,
+                     CheckerContext &C) const;
+  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  ProgramStateRef checkPointerEscape(ProgramStateRef State,
+                                     const InvalidatedSymbols &Escaped,
+                                     const CallEvent *Call,
+                                     PointerEscapeKind Kind) const;
+
+  int getEOFValue() const { return EOFValue; }
+
+  /// Determine the "known value" (if a constant) of an Expr.
+  static const llvm::APSInt *getKnownConstantVal(CheckerContext &C,
+                                                 const Expr *E) {
+    Optional<SVal> ConstantVal = C.getSValBuilder().getConstantVal(E);
+    if (ConstantVal)
+      return C.getSValBuilder().getKnownValue(C.getState(), *ConstantVal);
+    return nullptr;
+  }
+
+private:
+  EOFOrNegativeErrorReturn CheckForEOFOrNegative{this};
+
+  CallDescriptionMap<FnInfo> CheckedFunctions = {
+      {{"fputs", 2}, FnInfo{&CheckForEOFOrNegative}},
+      {{"fputws", 2}, FnInfo{&CheckForEOFOrNegative}},
+  };
+
+  /// Numerical value of 'EOF' macro, will be updated if more information is
+  /// found.
+  int EOFValue = -1;
+};
+
+bool EOFOrNegativeErrorReturn::testBinOpForCheckStatement(
+    CheckerContext &C, QualType RetTy, const BinaryOperator *BinOp,
+    const Expr *OtherSideExpr, bool OtherSideIsRHS) const {
+  // Anything other than comparison is not an error check.
+  if (!BinOp->isComparisonOp())
+    return false;
+
+  bool NullFound = false;
+  bool EOFFound = false;
+  const llvm::APSInt *ValueToTestAgainst =
+      ErrorReturnChecker::getKnownConstantVal(C, OtherSideExpr);
+  if (ValueToTestAgainst) {
+    NullFound = ValueToTestAgainst->isNullValue();
+    EOFFound = ((*ValueToTestAgainst) ==
+                C.getSValBuilder().getBasicValueFactory().getValue(
+                    Checker->getEOFValue(), RetTy));
+  } else {
+    // If a function is called at the other side of comparison, assume that it
+    // returns a correct NULL or EOF value. For example: 'X == getEOFValue()'.
+    // It is not assumable that a NULL or EOF is obtained using any other
+    // expression.
+    NullFound = isa<CallExpr>(OtherSideExpr);
+    EOFFound = NullFound;
+  }
+
+  BinaryOperator::Opcode Op = BinOp->getOpcode();
+  if (!OtherSideIsRHS)
+    Op = BinaryOperator::reverseComparisonOp(Op);
+
+  switch (Op) {
+  case BO_EQ: // 'X == -1'
+  case BO_NE: // 'X != -1'
+    return EOFFound;
+  case BO_LT: // 'X < 0'
+    return NullFound;
+  case BO_GE: // 'X >= 0'
+    return NullFound;
+  case BO_LE: // 'X <= -1'
+    return EOFFound;
+  case BO_GT: // 'X > -1'
+    return EOFFound;
+  default:
+    return false;
+  }
+
+  return false;
+}
+
+/// Result of the ErrorCheckTestStmtVisitor.
+enum VisitResult {
+  // Value use found that is not an error check.
+  NoCheckUseFound,
+  // An error check was found.
+  CheckFound,
+  // Assignment (like) condition was found (something that returns the same
+  // value as `Child`), the check should continue on upper level of the
+  // expression (with the current parent as new child).
+  ContinueAtParentStmt,
+  // Value appeared in any other way.
+  StopExamineNoError
+};
+
+/// Determine if a statement is an error-check for a return value.
+/// The statement to check is stored in `Child`.
+/// This can be the actual load statement that uses the return value,
+/// or a parent statement of it that stands for the same value.
+/// See `ErrorReturnChecker::checkAccess` for the calling algorithm.
+// FIXME: Provide description about the found problem.
+// Something like "unchecked value passed to system function call",
+// "value compared to ... instead of EOF", "unchecked value used in expression".
+class ErrorCheckTestStmtVisitor
+    : public ConstStmtVisitor<ErrorCheckTestStmtVisitor, VisitResult> {
+  CheckerContext &C;
+  /// Currently examined child statement.
+  const Stmt *Child;
+  /// Data about function whose return value is checked.
+  const FunctionCallData *Call;
+  const ParentMap &PM;
+
+  /// Check if `Child` is a child of `Parent` in `ParentMap`.
+  bool isChildOf(const Stmt *Parent) const {
+    for (const Stmt *C = Child; C; C = PM.getParent(C))
+      if (C == Parent)
+        return true;
+    return false;
+  };
+
+public:
+  ErrorCheckTestStmtVisitor(CheckerContext &C, const Stmt *Child,
+                            const FunctionCallData *Call)
+      : C(C), Child(Child), Call(Call),
+        PM(C.getLocationContext()->getParentMap()) {}
+
+  VisitResult VisitBinaryOperator(const BinaryOperator *BO) {
+    // Try to find the child expression on one side of the operator.
+    Expr *OtherS = nullptr;
+    for (const Stmt *P = Child; P && !OtherS; P = PM.getParent(P)) {
+      if (P == BO->getLHS())
+        OtherS = BO->getRHS();
+      else if (P == BO->getRHS())
+        OtherS = BO->getLHS();
+    }
+    assert(OtherS && "Invalid parent at binary operator.");
+
+    BinaryOperatorKind Op = BO->getOpcode();
+    if (Op == BO_Assign) {
+      // Value of child is transferred to parent, can continue with parent.
+      assert(OtherS == BO->getLHS() && "Loaded value not on assignment RHS.");
+      return ContinueAtParentStmt;
+    }
+
+    // `Child` (value to check) appears in the binary operator.
+    // Perform a specific check on the binary operator to determine if it is
+    // an error-check statement.
+    if (Call->Info->ErrorReturnKind->testBinOpForCheckStatement(
+            C, Call->Info->RetTy, BO, OtherS, OtherS == BO->getRHS()))
+      return CheckFound;
+    // Value appears in binary operator in other way than error check.
+    return NoCheckUseFound;
+  }
+
+  VisitResult VisitCallExpr(const CallExpr *CE) {
+    const FunctionDecl *CalledF = C.getCalleeDecl(CE);
+    SourceLocation Loc = CalledF->getLocation();
+    // Check if system function is called.
+    // It is assumed that any system function does not accept value that is
+    // an error return value from other (system) function.
+    // In other words error check is required before system function is called
+    // with the value.
+    // FIXME: There may be exceptions (like 'strerror').
+    if (Loc.isValid() && C.getSourceManager().isInSystemHeader(Loc))
+      return NoCheckUseFound;
+    // Non-system function call is no error, the call may be inlined and can
+    // contain error check.
+    return StopExamineNoError;
+  }
+
+  VisitResult VisitCStyleCastExpr(const CStyleCastExpr *CE) {
+    // Do not warn if explicit cast to void.
+    if (CE->getType().getCanonicalType() == C.getASTContext().VoidTy)
+      return CheckFound;
+    return NoCheckUseFound;
+  }
+
+  // Value appears in other expression.
+  VisitResult VisitExpr(const Stmt *S) { return NoCheckUseFound; }
+
+  VisitResult VisitDeclStmt(const DeclStmt *Decl) {
+    // Value is used at initialization.
+    return StopExamineNoError;
+  }
+
+  VisitResult VisitIfStmt(const IfStmt *S) {
+    if (isChildOf(S->getCond()))
+      return NoCheckUseFound;
+    return StopExamineNoError;
+  }
+
+  VisitResult VisitForStmt(const ForStmt *S) {
+    if (isChildOf(S->getInit()) || isChildOf(S->getCond()) ||
+        isChildOf(S->getInc()))
+      return NoCheckUseFound;
+    return StopExamineNoError;
+  }
+
+  VisitResult VisitDoStmt(const DoStmt *S) {
+    if (isChildOf(S->getCond()))
+      return NoCheckUseFound;
+    return StopExamineNoError;
+  }
+
+  VisitResult VisitWhileStmt(const WhileStmt *S) {
+    if (isChildOf(S->getCond()))
+      return NoCheckUseFound;
+    return StopExamineNoError;
+  }
+
+  // Value appears in other statement.
+  VisitResult VisitStmt(const Stmt *S) { return StopExamineNoError; }
+};
+
+} // end anonymous namespace
+
+REGISTER_MAP_WITH_PROGRAMSTATE(FunctionCallDataMap, SymbolRef, FunctionCallData)
+
+void ErrorReturnChecker::initialize(CheckerManager &Mgr) {
+  Optional<int> EOFExpanded = tryExpandAsInteger("EOF", Mgr.getPreprocessor());
+  if (EOFExpanded)
+    EOFValue = *EOFExpanded;
+}
+
+void ErrorReturnChecker::checkAccess(CheckerContext &C, ProgramStateRef State,
+                                     const Stmt *LoadS, SymbolRef CallSym,
+                                     const FunctionCallData *Call) const {
+  const ParentMap &PM = C.getLocationContext()->getParentMap();
+
+  while (LoadS) {
+    const Stmt *ParentS = PM.getParentIgnoreParenCasts(LoadS);
+
+    ErrorCheckTestStmtVisitor FindErrorCheck{C, LoadS, Call};
+    switch (FindErrorCheck.Visit(ParentS)) {
+    case NoCheckUseFound: {
+      if (!BT_UncheckedUse)
+        BT_UncheckedUse.reset(new BuiltinBug(
+            this, "Use of unchecked return value",
+            "Use of return value that was not checked for error"));
+
+      SourceRange CallLocation = Call->CallLocation;
+      State = State->remove<FunctionCallDataMap>(CallSym);
+
+      ExplodedNode *N = C.generateNonFatalErrorNode(State);
+      if (!N) {
+        C.addTransition(State);
+        return;
+      }
+
+      auto Report = std::make_unique<PathSensitiveBugReport>(
+          *BT_UncheckedUse, BT_UncheckedUse->getDescription(), N);
+      Report->addRange(CallLocation);
+      // FIXME: Provide information about source of CallSym.
+      C.emitReport(std::move(Report));
+
+      return;
+    }
+    case CheckFound:
+      // A correct error check was found, remove from state.
+      State = State->remove<FunctionCallDataMap>(CallSym);
+      C.addTransition(State);
+      return;
+
+    case ContinueAtParentStmt:
+      // Continue checking at upper level (check with result of assignment).
+      LoadS = ParentS;
+      continue;
+
+    case StopExamineNoError:
+      C.addTransition(State);
+      return;
+    };
+  }
+}
+
+ProgramStateRef ErrorReturnChecker::processEscapedParams(
+    CheckerContext &C, const CallEvent &Call, ProgramStateRef State) const {
+  for (unsigned int I = 0, E = Call.getNumArgs(); I < E; ++I) {
+    SVal V = Call.getArgSVal(I);
+    SymbolRef Sym = V.getAsSymbol();
+    if (Sym) {
+      State = State->remove<FunctionCallDataMap>(Sym);
+    }
+  }
+  return State;
+}
+
+const FnInfo *
+ErrorReturnChecker::findFunctionToCheck(CheckerContext &C,
+                                        const CallEvent &Call) const {
+  const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+
+  if (!FD || FD->getKind() != Decl::Function)
+    return nullptr;
+
+  if (!Call.isGlobalCFunction() || !Call.isInSystemHeader())
+    return nullptr;
+
+  const FnInfo *Fn = CheckedFunctions.lookup(Call);
+  if (!Fn)
+    return nullptr;
+
+  // The call should have a symbolic return value to analyze it.
+  SVal RetSV = Call.getReturnValue();
+  if (RetSV.isUnknownOrUndef())
+    return nullptr;
+  SymbolRef RetSym = RetSV.getAsSymbol();
+  if (!RetSym)
+    return nullptr;
+
+  // Lazy-init the return type when the function is found.
+  if (Fn->RetTy.isNull())
+    Fn->RetTy = FD->getReturnType();
+
+  return Fn;
+}
+
+void ErrorReturnChecker::checkPostCall(const CallEvent &Call,
+                                       CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+
+  State = processEscapedParams(C, Call, State);
+
+  const FnInfo *Fn = findFunctionToCheck(C, Call);
+  if (!Fn) {
+    C.addTransition(State);
+    return;
+  }
+
+  SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
+
+  FunctionCallData CallData{Fn, Call.getSourceRange()};
+  State = State->set<FunctionCallDataMap>(RetSym, CallData);
+
+  checkAccess(C, State, Call.getOriginExpr(), RetSym, &CallData);
+}
+
+void ErrorReturnChecker::checkLocation(SVal L, bool IsLoad, const Stmt *S,
+                                       CheckerContext &C) const {
+  if (!IsLoad)
+    return;
+  if (L.isUnknownOrUndef())
+    return;
+
+  auto Location = L.castAs<DefinedOrUnknownSVal>().getAs<Loc>();
+  if (!Location)
+    return;
+
+  ProgramStateRef State = C.getState();
+  SymbolRef Sym = State->getSVal(*Location).getAsSymbol();
+  if (!Sym)
+    return;
+
+  const FunctionCallData *Call = State->get<FunctionCallDataMap>(Sym);
+  if (!Call)
+    return;
+
+  checkAccess(C, State, S, Sym, Call);
+}
+
+ProgramStateRef ErrorReturnChecker::checkPointerEscape(
+    ProgramStateRef State, const InvalidatedSymbols &Escaped,
+    const CallEvent *Call, PointerEscapeKind Kind) const {
+  for (InvalidatedSymbols::const_iterator I = Escaped.begin(),
+                                          E = Escaped.end();
+       I != E; ++I) {
+    SymbolRef Sym = *I;
+    State = State->remove<FunctionCallDataMap>(Sym);
+  }
+  return State;
+}
+
+void ento::registerErrorReturnChecker(CheckerManager &Mgr) {
+  auto *Checker = Mgr.registerChecker<ErrorReturnChecker>();
+  Checker->initialize(Mgr);
+}
+
+bool ento::shouldRegisterErrorReturnChecker(const CheckerManager &Mgr) {
+  return true;
+}

clang/test/Analysis/Inputs/system-header-simulator.h

 
 FILE *fopen(const char *path, const char *mode);
 FILE *tmpfile(void);
 FILE *freopen(const char *pathname, const char *mode, FILE *stream);
 int fclose(FILE *fp);
 size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
 size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
 int fputc(int ch, FILE *stream);
+int fputs(const char *restrict s, FILE *restrict stream);
 int fseek(FILE *__stream, long int __off, int __whence);
 long int ftell(FILE *__stream);
 void rewind(FILE *__stream);
 int fgetpos(FILE *stream, fpos_t *pos);
 int fsetpos(FILE *stream, const fpos_t *pos);
 void clearerr(FILE *stream);
 int feof(FILE *stream);
 int ferror(FILE *stream);

 typedef void (*xpc_finalizer_t)(void *value);
 void xpc_connection_set_context(xpc_connection_t connection, void *context);
 void xpc_connection_set_finalizer_f(xpc_connection_t connection, xpc_finalizer_t finalizer);
 void xpc_connection_resume(xpc_connection_t connection);
 
 //The following are fake system header functions for generic testing.
 void fakeSystemHeaderCallInt(int *);
 void fakeSystemHeaderCallIntPtr(int **);
+void fakeSystemHeaderCallIntVal(int);
 
 // Some data strauctures may hold onto the pointer and free it later.
 void fake_insque(void *, void *);
 typedef struct fake_rb_tree { void *opaque[8]; } fake_rb_tree_t;
 void fake_rb_tree_init(fake_rb_tree_t *, const void *);
 void *fake_rb_tree_insert_node(fake_rb_tree_t *, void *);
 
 typedef struct __SomeStruct {

clang/test/Analysis/error-return.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.unix.ErrorReturn -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+FILE *file();
+
+void test_EOFOrNeg_LT_Good() {
+  if (fputs("str", file()) < 0) {
+  }
+}
+
+void test_EOFOrNeg_LT_Bad() {
+  if (fputs("str", file()) < -1) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_GT_Good() {
+  if (fputs("str", file()) > -1) {
+  }
+}
+
+void test_EOFOrNeg_GT_Bad() {
+  if (fputs("str", file()) > 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_LE_Good() {
+  if (fputs("str", file()) <= -1) {
+  }
+}
+
+void test_EOFOrNeg_LE_Bad() {
+  if (fputs("str", file()) <= 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_GE_Good() {
+  if (fputs("str", file()) >= 0) {
+  }
+}
+
+void test_EOFOrNeg_GE_Bad() {
+  if (fputs("str", file()) >= -1) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_EQ_Good() {
+  if (fputs("str", file()) == -1) {
+  }
+}
+
+void test_EOFOrNeg_EQ_Bad() {
+  if (fputs("str", file()) == 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_NE_Good() {
+  if (fputs("str", file()) != -1) {
+  }
+}
+
+void test_EOFOrNeg_NE_Bad() {
+  if (fputs("str", file()) != 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_EQ_BadVal() {
+  if (fputs("str", file()) == -2) { // expected-warning{{Use of return value that was not checked}}
+  }
+  if (fputs("str", file()) == 1) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_VarAssign() {
+  int X = fputs("str", file());
+  if (X != 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_VarAssignInCond() {
+  int X;
+  if ((X = fputs("str", file())) != 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_VarAssign1() {
+  int X = fputs("str", file());
+  int Y = X;
+  if (Y != 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void badcheck(int X) {
+  if (X == 0) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_Call() {
+  int X = fputs("str", file());
+  badcheck(X);
+}
+
+void test_EOFOrNeg_Syscall() {
+  int X = fputs("str", file());
+  fakeSystemHeaderCallIntVal(X);                    // expected-warning{{Use of return value that was not checked}}
+  fakeSystemHeaderCallIntVal(fputs("str", file())); // expected-warning{{Use of return value that was not checked}}
+}
+
+void test_EOFOrNeg_Use_LNot() {
+  int X = fputs("str", file());
+  if (!X) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_Use_Add() {
+  int X = fputs("str", file());
+  int Y = X + 1; // expected-warning{{Use of return value that was not checked}}
+}
+
+void test_EOFOrNeg_If() {
+  int X = fputs("str", file());
+  if (X) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_IfCond() {
+  if (fputs("str", file())) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_ForInit() {
+  for (fputs("str", file());;) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_ForCond() {
+  for (; fputs("str", file());) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_ForInc() {
+  for (;; fputs("str", file())) { // expected-warning{{Use of return value that was not checked}}
+  }
+}
+
+void test_EOFOrNeg_DoCond() {
+  do {
+  } while (fputs("str", file())); // expected-warning{{Use of return value that was not checked}}
+}
+
+void test_EOFOrNeg_WhileCond() {
+  while (fputs("str", file())) { // expected-warning{{Use of return value that was not checked}}
+  };
+}
+
+void unknown1(int);
+
+void test_EOFOrNeg_EscapeCall() {
+  int X = fputs("str", file());
+  unknown1(X);
+  int Y = X + 1;
+}
+
+int GlobalInt;
+
+void test_EOFOrNeg_EscapeGlobalAssign() {
+  GlobalInt = fputs("str", file());
+  int X = GlobalInt + 1;
+}
+
+void test_EOFOrNeg_NoErrorAfterGoodCheck() {
+  int X = fputs("str", file());
+  if (X < 0) {
+  }
+  if (X < 1) {
+  }
+}
+
+int getEOFValue();
+
+void test_EOFOrNeg_CompareToFunction() {
+  int R = fputs("str", file());
+  if (R == getEOFValue())
+    return;
+}
+
+void test_EOFOrNeg_CompareToOther() {
+  int R = fputs("str", file());
+  if (R == 1 + getEOFValue()) // expected-warning{{Use of return value that was not checked}}
+    return;
+}
+
+void test_ValueNotStored() {
+  // FIXME: Warning is expected here
+  fputs("str", file());
+}
+
+void test_UnusedValue() {
+  // FIXME: Warning is expected here
+  // (but this is an unused variable too)
+  int R = fputs("str", file());
+}
+
+void test_CastToVoidImmediately() {
+  (void)fputs("str", file()); // no warning
+}
+
+void test_CastToVoidLater() {
+  int R = fputs("str", file());
+  (void)R; // no warning
+}