Try to stop range overflow in forwardRange()

[jpyllman]

In some cases, like when you have an assignment in an if condition, the loop in PathAnalysis::forwardRange() will continue outside the condition it is supposed to check. That is because the assignment check will put and on the token ), which is also the end token, and then do a tok->next() and check against endToken. This is an extra check if we already on the endToken.

This is probably a solution for trac item #10933. Because that example contains 'many' if statements with assignment in the condition. This makes the run very recursive.

All tests sill passes.

[orbitcowboy]

Thanks! Do you see any chance to add a regression test?

[jpyllman]

I am not sure how I could make a test for this. But I guess forwardRange() need some tests to make sure some other case also ends up overstepping the end of the range.

[pfultz2]

It would be better to use precedes(tok, endToken) as the stopping condition in the for loop instead. Similar to how it is done here:

https://github.com/danmar/cppcheck/blob/main/lib/forwardanalyzer.cpp#L552

[jpyllman]

That sure could be an option. At least for the case my change was applied. But I still wonder what would happen if the endToken is the last token, and tok->next() would put you on the files first token. Which the original line was for.

For correct code in the token list this is not a big risk. But for odd/strange/faulty code, I have to little experience what kind of problem these tokAt() and linkAt() can create.

[jpyllman]

Even in the code you link to there is an extra check for the cyclic case.

[jpyllman]

I like the idea that all 'similar' loops use the same checks. Only thing I have against it is that it adds CPU cost with a function call. But hopefully it adds safety.

[jpyllman]

Another question is. Should I also change the tok->next() == start to the same check in the loop you showed. And also throw an exception?

[jpyllman]

Thanks! Do you see any chance to add a regression test?

The fix in this PR does not really fix any existing test that fails. Actually all existing tests in CheckStl::invalidContainer() succeed before and after this change. What this change fixes is that an assignment in an if statement will overflow and possible make all coming if statements with assignments create a recursion that is not needed. So the test file in #10933 goes from taking 41 minutes to less then a second on my machine with the same result.