Fixed Inappropriate Logical Expression

[fazledyn-or]

What do these changes do?

• Removed a break statement inside the 'finally' block of a try-catch statement. Using 'break' or 'continue' inside a finally block can suppress the propagation of unhandled exception.

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Details

While triaging your project, our bug fixing tool generated the following message(s)-

In file: web_protocol.py, method: start, a break statement is used inside a finally block. This will discard the temporarily saved unhandled exception which was raised inside the try, else or except block. iCR suggested that the break statement should not be used inside the finally block.

How It Works

Incorrect Use
You can have a look at the example below-

foo = [1, 2, 3, 4, 5]
bar = None
for _ in range(5):
    try:
        # IndexError
        bar = foo[10]
        print("Inside try block")
    except:
        print("Inside except block")
        # raising another exception
        bar = foo[100]
    else:
        print("Inside else block")
        # let's try another exc
        bar = foo[100]
    finally:
        break

Inside except block

Correct Use

foo = [1, 2, 3, 4, 5]
bar = None
for _ in range(5):
    try:
        # IndexError
        bar = foo[10]
        print("Inside try block")
    except:
        print("Inside except block")
        # raising another exception
        bar = foo[100]
    else:
        print("Inside else block")
        # let's try another exc
        bar = foo[100]
    finally:
        pass

Inside except block
Traceback (most recent call last):
  File "try-catch-break.py", line 13, in <module>
    bar = foo[10]
IndexError: list index out of range

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "try-catch-break.py", line 18, in <module>
    bar = foo[100]
IndexError: list index out of range

From these two examples, we can see that if break is used in the finally block, it ignores any exceptions that takes place in except or else. But if pass is used, or finally block isn't used, the exceptions are reported correctly. As a result, the correct use is not to use break or continue inside the finally block.

I've removed the else and break statement from the code since it'll lead to a better understanding of any exceptions that may take place during code execution.

CLA Requirements

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

All contributed commits are already automatically signed off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).
- Git Commit SignOff documentation

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

[Dreamsorcerer]

Good catch, I'm just trying to figure out if the transport.close() below should be run in the case of a BaseException that is uncaught..
@bdraco Do you think it's fine like this?

[bdraco]

I think this is a risk of the transport not getting closed on BaseException. I'm not sure what that exception would be though.

maybe something like...

diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py
index 3cefd5df..f2dcf5aa 100644
--- a/aiohttp/web_protocol.py
+++ b/aiohttp/web_protocol.py
@@ -589,6 +589,9 @@ class RequestHandler(BaseProtocol):
             except Exception as exc:
                 self.log_exception("Unhandled exception", exc_info=exc)
                 self.force_close()
+            except BaseException:
+                self.force_close()
+                raise
             finally:
                 if self.transport is None and resp is not None:
                     self.log_debug("Ignored premature client disconnection.")

[Dreamsorcerer]

I'm not sure what that exception would be though.

Well, in most cases, it'll probably be a KeyboardInterrupt, which may or may not lead to the program exiting..

maybe something like...

Seems fine with me.

[Dreamsorcerer]

Currently, the tests don't even complete, so the current change doesn't seem correct. Maybe with the proposed change it will work correctly..

[Dreamsorcerer]

Tests are still not completing. @fazledyn-or Any interest in finishing this PR?

[fazledyn-or]

Tests are still not completing. @fazledyn-or Any interest in finishing this PR?

It's been long since I had worked on it, and yeah- I'll work toward finishing the PR. However, I don't understand the reason behind introducing an empty raise.

[Dreamsorcerer]

I think in the original code if a BaseException happened, it could trigger the break, which then leads to closing the transport outside the exception handler. We shouldn't be suppressing a BaseException though, so to be safe we want to force close on a BaseException and then reraise it.

Looking over that code again, it feels like that block of logic should maybe not be in the finally block...

[mikezolo]

One of the best based and explicitly explained pull requests I've read.

And the fix is indeed substantial.

[webknjaz]

FWIW a changelog entry should explain the high-level effect for the end-users. Throwing internal implementation details at them doesn't seem useful..

[webknjaz]

@fazledyn-or is this still on your radar? Unfortunately, we cannot merge this until it stops shuttering the CI...