Fix #1473: warn when fclose() is used as a while loop condition#8444
Fix #1473: warn when fclose() is used as a while loop condition#8444francois-berder wants to merge 5 commits intocppcheck-opensource:mainfrom
Conversation
|
| const Token* bodyEnd = nullptr; | ||
| const Token* bodyStart = nullptr; | ||
|
|
||
| if (Token::simpleMatch(loopTok->previous(), "}")) { // Handle do-while loops |
There was a problem hiding this comment.
it doesn't have to be a do-while when previous token is a "}".
| operation = Filepointer::Operation::CLOSE; | ||
|
|
||
| // #1473 Check if fclose is in a while loop condition | ||
| if (fileTok) { |
There was a problem hiding this comment.
the old code does not use AST so it doesn't handle well when the parameter is some expression.
imagine:
while (i < 10 && fclose(f[i++]) {}
It is ugly code. I suggest you check that fileTok is just a simple variable token.
| } | ||
|
|
||
| // Do not trigger a warning if the loop always exits or if the file is opened again in the loop. | ||
| if (!isReturnScope(bodyEnd, mSettings->library) && Token::findmatch(bodyStart, "%var% = fopen|freopen", bodyEnd, fileTok->varId()) == nullptr) |
There was a problem hiding this comment.
if there is any f = some_function(); in the loop we shouldn't warn. I suggest:
| if (!isReturnScope(bodyEnd, mSettings->library) && Token::findmatch(bodyStart, "%var% = fopen|freopen", bodyEnd, fileTok->varId()) == nullptr) | |
| if (!isReturnScope(bodyEnd, mSettings->library) && Token::findmatch(bodyStart, "%var% =", bodyEnd, fileTok->varId()) == nullptr) |
…oop condition Using fclose() as a while loop condition closes the file on every iteration and operates on an already-closed file handle from the second iteration onward. Signed-off-by: Francois Berder <fberder@outlook.fr>
…while loop condition
…d as a while loop condition
… is used as a while loop condition
…close() is used as a while loop condition
francois-berder
force-pushed
the
pr-1473
branch
from
3491db9 to
28f2247
Compare
| if (fileTok && fileTok->isVariable()) { | ||
| const Token* tmp = tok->astParent(); | ||
| const Token* loopTok = nullptr; | ||
| while (tmp) { |
There was a problem hiding this comment.
could we use Token::astTop() here instead of the loop?
| const Token* bodyEnd = nullptr; | ||
| const Token* bodyStart = nullptr; | ||
|
|
||
| if (Token::simpleMatch(loopTok->previous(), "}") && Token::simpleMatch(loopTok->linkAt(-1)->previous(), "do")) { // Handle do-while loops |
There was a problem hiding this comment.
I would spontanously prefer that we look at the scope type.
if (Token::simpleMatch(loopTok->previous(), "}") &&
loopTok->previous()->scope()->scopeType == Scope::ScopeType::eDo)
| " FILE *a = fopen(\"aa\", \"r\");\n" | ||
| " do {} while (fclose(a));\n" | ||
| "}"); | ||
| ASSERT_EQUALS("[test.cpp:3:18]: (error) Used file that is not opened. [useClosedFile]\n", errout_str()); |
There was a problem hiding this comment.
I am not sure about the message. The file is opened but not in the loop. The first loop iteration it works fine. I don't know how I would like to phraze it... here is a rough idea:
File handle 'a' can be closed more than once in loop. 'a' is not opened in the loop.
Feel free to suggest something better.
2 participants
Using fclose() as a while loop condition closes the file on every iteration and operates on an already-closed file handle from the second iteration onward.