Removed C++ support.
Other small changes.

Updated check for system function.
Updated documentation.
Added more test cases.

I plan to add the option for extended set of asynchronous-safe functions (defined by the POSIX list) in a next patch. There is other possible improvement to check at least something of the criteria listed here for C++17 .

Added support for C++ code.
Improved detection of 'signal' function.
Simplified collection of called functions.

I do not know if it got overlooked but there is another patch D81061 that removes the crash.

Results for 'duckdb': link

Here are the results with this checker on project 'emacs`.
Still more evaluation is needed.

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

Ping

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

Changed checker messages, reformatted text, rebase.

Added entry to release notes and fixed wrong comment in test headers.

Probably it should be tested how this change affects CTU analysis (function body is checked now).

It looks like that the clang-tidy/add_new_check.py script does not work correctly, at least it "corrupts" the list.rst file, and creates files with no newline at end.

• Code formatting fixes.
• Updated description.
• Improved CalledFunctionsCollector.

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition. The return value is constrained by StreamChecker too but it splits the error (if returned value is less that arg 3) and non-error cases into separate branches. I think this causes no problem because it will refine the assumption made here (if this assumption is made first) or the assumption here has no effect (if the split happened already).

LGTM.

Looks good now.

I do not know how these changes can appear:

The fix is probably OK but I could not find out what causes the problem in this case and not in other (similar) ones.
Why is not possible to assume SVB.evalEQ(State, DynSize, *ArraySizeNL) to true:
DynSize: extent_$1{e}
*ArraySizeNL: 8 U64b
The problem occurs likely not at the first iteration of the loop. Probably something is "messed up" in the state.

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.

Something like this can be added to StreamChecker but it has changed in many places so a new review would be needed. The main problem is with this fgetc check is that it would be good to check the standard stream version getc too, but that is hard to implement because global nature of the standard streams (stdin).

This patch is here for code reference only. Although the whole checker does not work safe in this way the idea behind this approach can be useful.

It is better now, but the refactoring change should be in a separate revision (it affects the other already added functions too).

Even some macro-like construct can improve readability. The current way of adding functions is source of copy-paste errors because more things are repeated, these should be written only once. And the ifs for the existence of types can be eliminated (automated) somehow. So the final way of specifying the functions is to only list (get) the types first, then add the functions with name, arguments and constraints in a way that nothing needs to be repeated and computable parts are done automatically.

There is a TODO comment at the error message, it needs improvement. The current form is still something for developers, not for end users. For final version I would accept textual descriptions (as mentioned above), not names like "BufferSize" and words like "ArgN" inside it.

Nice!

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

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

More results in CodeChecker: emacs_errorreturn

• Rebase
• Changed testBinOpForCheckStatement

Rebase (bug category is LogicError).

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

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

Preserve original error messages.

NFC changes, added some tests.

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

Really I am still not totally familiar how the checkers work and if it is good to have these function names. I was thinking about any checker that needs a string length information. It could get the data from CStringChecker using a "get" function or test if there is data at all. In this case it may happen that this get function modifies state or computes the length at that point? And how do the set operation work, it only stores a value computed by the calling code? And the create operation then does the computation and set together? The functions look not symmetrical, the set and create takes a MemRegion but the get takes a SVal. The create function sets the length for the passed memory region but makes no tests on it like the set function (can we call the set function from the create?).

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)

The category string seems to be not really important, better not to change it.

Changed MemoryError back to LogicError.
Improved placement and text of changed comment.

Crash is fixed here: D84843

Then the current solution is good, print always end of the bug path.
A change to the bug reporting component was made that caused reported position of bugs to change. New is the end of the path, old is the location set by the checker (BugReport::getLocation value). The location is often same as end of the path. The RefLeakReport has a special getLocation function and there is a difference between this location (that is the allocation site) and the end of the path.

From the beginning on in this patch I assumed that the location of the bug report should be the end of the bug path. But this is probably a wrong approach, specially if the bug report has uniqueing location. In that case the uniqueing location may be a better place for the bug report. Specially for resource leak it is not bad if the report is located at the allocation site (still there is a bug path that shows the full path and place of leak). So another approach can be to allow only bug equivalence classes where the reports have same locations. If this is not true the checker should be fixed that generates the reports.

Change of the existing error messages and bug category is not necessary but it would be improvement because more uniform error messages. (And not the "logic error" is the best category for this case, if this value is used at all. Other checkers have bad values too.)

Not sure if this is a good solution. There are few tests that test the undefined reference case. But the added code should work the same way as in the null pointer case?

Fixed failed tests because change of bug category from "Logic error" to "Memory error".

Change column in malloc-plist test because code was reformatted.

Every other test failure comes from RetainCount checker except malloc-plist.c.

Maybe the assert b.hasValue() is related to the problems in D83115 or D83961? The changes in those affect the same functions.

NFC code improvements.
Detect expressions in conditions.

The problem in D83120 is fixed by this patch.
What I figured out from the code is that BugReporter::FlushReport calls findReportInEquivalenceClass that returns a report that has not necessary the shortest path. That report is get from the passed bug report class and a different instance is returned if the uniqueing is turned on or off. This report is used to fill the last location in the bug path, if the bug path is empty. So the location in the bug path changes dependent on the setting of uniqueing. The bug path here is empty if the analyzer-output is set to minimal (or not specified). Even in the minimal output case, function PathDiagnosticBuilder::generate is called if it is a path sensitive case. In that function the shortest path is used. So instead of returning an empty path from that function (what is filled later by FlushReport) the last part of the path can be filled here (and use the correct source location from the shortest path).
The problem can happen if not the report with shortest path is returned from findReportInEquivalenceClass, and analyzer-output is set to minimal or default. If the output is not minimal, the bug path is filled by the PathDiagnosticBuilder::generate with correct locations. So changing the output type can result in change of the warning location.
Other way to get the problem is change only the uniqueing setting in bug type and use minimal (or default) output. Then the example report can change and because this example report is used to get the bug location the location can change too (if the equivalence class contains paths with different end locations).

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.

Rebase to master.

Fixed formatting.

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
}

Future improvement: Split ASTImporterTest.cpp into smaller parts. It is still too large.

Checking notes in test f_leak_2.

Big part of the test failures is with the osx.cocoa.RetainCount checker. Only a small part of the errors is fixed now.

Fixed some source location changes in tests.
Re-added check for empty path.