github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.48k stars 1.49k forks source link

LGTM.com - false positive #5910

Open webbnh opened 3 years ago

webbnh commented 3 years ago

LGTM reports as unreachable a line following a finally clause attached to a try block containing a seemingly-infinite loop which is exited via a side-effect of a signal handler.

I'll grant that there are a number of ways that execution could fail to reach the flagged statement, but there is at least one way which I think it can.

See non-infinite loop.

webbnh commented 3 years ago

Here's another example of what is presumably the same problem.

tausbn commented 3 years ago

I don't think the first one is a false positive. Consider the following piece of code (roughly isomorphic to the first bit of code you linked to):

if True:
    try:
        1 / 0
    finally:
        print("Inside finally block")
    print("After finally block")

If you try running the above code, you'll see that it prints Inside finally block and then spits out a ZeroDivisionError, but doesn't print the After finally block line. This is because after the finally block has been run, it re-raises the appropriate exception (unless the finally block itself discards the exception, e.g. by invoking return, break, or continue).

For the second example you link to, it seems more likely that we're simply not observing that terminate may have its value modified, and hence break out of the infinite loop. In this case, however, it does appear that this is a false positive.

As this is not a security-related query, and as our main focus at the moment is on improving our security analysis, I can't give you an indication of when this false positive will be fixed. In the meantime, if the alert bothers you, you can suppress it as described in the LGTM.com help pages.

webbnh commented 3 years ago

@tausbn, the first case boils down to code like this:

if True:
    try:
        terminate = False
        while not terminate:
            signal.pause()
    finally:
        pass  # For loop....
    sys.exit(0)  # Unreachable?

If there is no exception, then control should drop out of the bottom of the finally clause and reach the next statement, but only if LGTM can determine that it can escape the while loop....

I can't give you an indication of when this false positive will be fixed.

No worries -- I've already added the suppression comment. :-)

tausbn commented 3 years ago

@tausbn, the first case boils down to code like this:

Ah you're right, thanks! I misread the original code (and simplified it a bit too much, whoops). I agree that this appears to be the same underlying issue, then. :+1:

webbnh commented 3 years ago

No worries -- it took me quite awhile to figure out myself. :-)