Fix #958: warn when feof() is used as a while loop condition

[francois-berder]

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.

[chrchr-github]

How does the check handle do ... while loops? Should we skip those?
There are obviously other ways to check the return value of feof, e.g. using ==. Another extension would be handling of for loops, but maybe that could be added later. @danmar

[danmar]

thank you for adding this checker! it's good work. small and simple. just try to make it more precise.
please add a line in the release notes.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[danmar]

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.

[francois-berder]

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

[danmar]

The single CI failure seems to be a timeout unrelated to this PR.

That CI test is a bit flaky. :-(

So if that happens some time again you can assume that it's not related.

[francois-berder]

@danmar I tried to address your concerns about the check so I added more bailouts. I also rebased the code on a more recent commit.

[francois-berder]

I also added do-while loops support.

[danmar]

I do not feel that findFileReadCall is implemented properly it only handles a few hard coded functions and if other functions are used then there can be false positives.
When analysis is inconclusive our philosophy is to have false negatives rather than false positives.

[francois-berder]

I do not feel that findFileReadCall is implemented properly it only handles a few hard coded functions and if other functions are used then there can be false positives. When analysis is inconclusive our philosophy is to have false negatives rather than false positives.

I understand your concern, nonetheless I think the combination of findFileReadCall and this bailout avoids FP at the cost of FN:

            // Bail out if fp is used outside of known file I/O functions.
            // If it is passed to an unknown function, reads may occur there.
            bool fpUsedElsewhere = false;
            for (const Token *t = bodyStart->next(); t && t != bodyEnd; t = t->next()) {
                if (t->varId() != fpVarId)
                    continue;
                const Token *p = t->astParent();
                while (p && p->str() == ",")
                    p = p->astParent();
                if (!p || !Token::Match(p->astOperand1(), "fgets|fgetc|getc|fread|fscanf|fprintf|fwrite|fputs|fputc|putc")) {
                    fpUsedElsewhere = true;
                    break;
                }
            }
            if (fpUsedElsewhere)
                continue;

This bailout is quite powerful and will clearly create a lot of false negatives.
Hence, there are two cases:

• if some code is using a different file read function outside the list "fgets|fgetc|getc|fread|fscanf", loopFileReadCallTok is NULL and the check bails out.
• if some file is read using "fgets|fgetc|getc|fread|fscanf" but the file pointer is referenced anywhere else (assignment, parameter to a function call), the check bails out

Do you have in mind a false positive that could be caused by this check?

[danmar]

Alright. We are in the process of making a release so the releasenotes will need to be adjusted. then I think we can merge this.

[francois-berder]

@danmar I solved the conflict in releasenotes.txt

[francois-berder]

It seemed that my rebase was wrong so I fixed it. I also updated man/checkers/wrongfeofUsage.md