Fix #958: warn when feof() is used as a while loop condition#8422
Fix #958: warn when feof() is used as a while loop condition#8422francois-berder wants to merge 4 commits intocppcheck-opensource:mainfrom
Conversation
|
How does the check handle |
|
thank you for adding this checker! it's good work. small and simple. just try to make it more precise. |
|
|
I hope some of those CI failures are relatively easy to fix. When you added a checker the checker count increased.. Can you please add a document that explains this checker in cppcheck/man/checkers folder. My long term goal is that all checkers will have a document there. |
… condition feof() only returns true after a read has already failed, causing the loop body to execute once more after the last successful read. Read errors also go undetected since feof() does not distinguish them from EOF. Signed-off-by: Francois Berder <fberder@outlook.fr>
…le loop condition
…s a while loop condition
… used as a while loop condition
|
@danmar I rebased the PR on top of main and fixed the issues you mentioned. I think this PR is now again ready for review. The single CI failure seems to be a timeout unrelated to this PR. |
| // Usage of feof is correct if a read happens before and within the loop. | ||
| // However, if we find a control flow statement in between the fileReadCall | ||
| // token and the while loop condition, then we bail out. | ||
| const Token *endCond = tok->linkAt(1); |
There was a problem hiding this comment.
I would prefer an explicit check somewhere that the code is as expected.
if (!Token::simpleMatch(endCond, ") {"))
continue;
| for (const Scope * scope : symbolDatabase->functionScopes) { | ||
| for (const Token *tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { | ||
| // TODO: Handle do-while and for loops | ||
| if (!Token::simpleMatch(tok, "while ( ! feof (")) |
There was a problem hiding this comment.
your code expects that the argument is just a variable. I like to make match patterns explicit..
Token::simpleMatch(tok, "while ( ! feof ( %var% )")
|
|
||
| // Bail out if we cannot identify file pointer | ||
| const int fpVarId = tok->tokAt(5)->varId(); | ||
| if (fpVarId == 0) |
There was a problem hiding this comment.
this does not identify the file pointer very well. varid can be nonzero in some such patterns:
feof(files[10])
feof(index + files);
feof(data.fp);
|
|
||
| static const Token* findFileReadCall(const Token *start, const Token *end, int varid) | ||
| { | ||
| const Token* found = Token::findmatch(start, "fgets|fgetc|getc|fread|fscanf (", end); |
There was a problem hiding this comment.
Imagine such function call:
skip_empty_lines(f);
If skip_empty_lines is not defined in the current TU (translation unit) then you need to bailout somehow so there are not false positives.
If skip_empty_lines is defined in the current TU you could look into that and determine if there is file read but I am not against that we bailout for that case also.
If you pass f to a function it's highly likely it is to read it.. why else would it be passed to the function since you use feof on f also..
That CI test is a bit flaky. :-( So if that happens some time again you can assume that it's not related. |
| const Token *endCond = tok->linkAt(1); | ||
| const Token *endBody = endCond->linkAt(1); | ||
|
|
||
| const Token *prevFileReadCallTok = findFileReadCall(scope->bodyStart, tok, fpVarId); |
There was a problem hiding this comment.
Sorry for misleading you but it doesn't matter if the file is read before the loop. What matters is the order of feof, read, use in the loop..
No matter if the file is read before the loop or not..
// this loop is safe
while (!feof(f)) {
puts(line); // use
fgets(line, sizeof(line), f); // read
}
// I guess this loop is "safe". just skip some number of lines..
while (!feof(f) && --count > 0) {
fgets(line, sizeof(line), f); // read
}
// you can warn about this:
while (!feof(f)) {
fgets(line, sizeof(line), f); // read
puts(line); // use
}
3 participants
feof() only returns true after a read has already failed, causing the loop body to execute once more after the last successful read. Read errors also go undetected since feof() does not distinguish them from EOF.