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 inappropriate length comparison #1940

Conversation

munahaf
Copy link
Contributor

@munahaf munahaf commented Aug 17, 2023

In file: nttcis.py, the comparison of collection length creates a logical short circuit. Since we are comparing two elements, the length should be greater than 1. This is moved to the outside if. The change should be reviewed so that it captures the intended business logic.

Sponsorship and Support:

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF)(https://openssf.org/): Project Alpha-Omega(https://alpha-omega.dev/). 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.

@Kami
Copy link
Member

Kami commented Aug 17, 2023

Thanks for the contribution.

The original logic indeed looks weird / wrong, but I didn't originally implement the code and I'm not sure about the logic and the reasoning behind it.

Having said that, some tests for that logic (including various edge cases) would be a good start (and of course making sure that existing tests / if any, still pass).

@munahaf
Copy link
Contributor Author

munahaf commented Aug 17, 2023

I am not familiar with the business logic of the code. Unfortunately, I would not be able to come up with the test cases. We are just following the anomaly as identified by a static analysis tool.

@Kami
Copy link
Member

Kami commented Aug 22, 2023

@munahaf Thanks.

Similar here and that's also why I don't feel that comfortable with merging this code - there are no tests and I don't fully understand the original developer reasoning.

@munahaf
Copy link
Contributor Author

munahaf commented Aug 22, 2023

At least the change I proposed appears less confusing and still retains the original logic of the code. Perhaps we can start with that.

Per git blame, the code was initiated by this effort: b19a165

What is the appropriate way of getting some stakeholders (committer, reviewer) involved?

@Kami
Copy link
Member

Kami commented Sep 13, 2023

@munahaf Agreed.

You could try pinging the person who originally added that code change (@snmpboy), but no idea if they are still involved with that provider.

Copy link
Member

@Kami Kami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think it's fine and safe to merge this change. And if we ever hear back from the person who originally implemented this change, we can make any adjustments then, if needed.

@codecov-commenter
Copy link

Codecov Report

Merging #1940 (87eda10) into trunk (b96bf0e) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##            trunk    #1940   +/-   ##
=======================================
  Coverage   83.18%   83.18%           
=======================================
  Files         353      353           
  Lines       81333    81332    -1     
  Branches     8579     8578    -1     
=======================================
  Hits        67656    67656           
+ Misses      10873    10872    -1     
  Partials     2804     2804           
Files Changed Coverage Δ
libcloud/common/nttcis.py 73.71% <0.00%> (+0.12%) ⬆️

@Kami Kami merged commit 69b922e into apache:trunk Sep 13, 2023
18 checks passed
asfgit pushed a commit that referenced this pull request Sep 13, 2023
@Kami Kami added this to the v3.9.0 milestone Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants