Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an inappropriate test expression to remove a logical short circuit #1559

Conversation

munahaf
Copy link
Contributor

@munahaf munahaf commented Oct 6, 2023

In file: processor.py, the comparison of Collection length creates a logical short circuit. The length of a collection is always greater than or equal to zero. So testing that a length is less than zero is always false.

I suggested that the Collection length comparison should be done without creating a logical short circuit.

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.

@heinezen heinezen force-pushed the Inappropriate_Logic-3processor.py9361182525541201156.diff branch from 9ec1c80 to 09e7d26 Compare October 6, 2023 20:33
@heinezen
Copy link
Member

heinezen commented Oct 6, 2023

Thanks for the fix. Can you do the following before we merge this?

  • You need to add your name to the copying.md file
  • Can you change the check to len(upgrade_effects) == 0? While this is considered unpythonic by some, it makes the check more readable.

munahaf added a commit to munahaf/openage that referenced this pull request Oct 6, 2023
@heinezen heinezen added area: assets Involved with assets (images, sounds, ...) bugfix Restores intended behavior labels Oct 6, 2023
TheJJ
TheJJ previously approved these changes Oct 7, 2023
@TheJJ
Copy link
Member

TheJJ commented Oct 7, 2023

the commit for your authorship isn't pushed to this pull request's branch yet.

@heinezen
Copy link
Member

heinezen commented Oct 8, 2023

Do you need help with any of this?

heinezen pushed a commit to munahaf/openage that referenced this pull request Oct 8, 2023
heinezen pushed a commit to munahaf/openage that referenced this pull request Oct 8, 2023
@heinezen heinezen force-pushed the Inappropriate_Logic-3processor.py9361182525541201156.diff branch from 85d9636 to 9e9b279 Compare October 8, 2023 15:56
@heinezen heinezen force-pushed the Inappropriate_Logic-3processor.py9361182525541201156.diff branch from 9e9b279 to 9422cbe Compare October 8, 2023 15:58
@heinezen
Copy link
Member

heinezen commented Oct 8, 2023

I've cherry picked your commit and added it to the correct branch. It should be mergeable now.

@TheJJ TheJJ merged commit 4773e70 into SFTtech:master Oct 8, 2023
8 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: assets Involved with assets (images, sounds, ...) bugfix Restores intended behavior
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants