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

fabtests: Address integer overflow coverity issues in fabtests #10142

Closed
wants to merge 1 commit into from

Conversation

Juee14Desai
Copy link
Contributor

There are 4 integer overflow issues in fabtests. All of them need a check that ensures integer overflow does not oocur.

  1. changes to harness.c: This modification includes a check to ensure that adding ret to m does not exceed SIZE_MAX.

  2. changes to shared.c: This modification includes a check before the addition to ensure that (rcvd + ret) or (sent + ret) does not exceed SIZE_MAX. If the addition would exceed SIZE_MAX, it sets err to an error value and breaks from the loop, preventing overflow.

There are 4 integer overflow issues in fabtests. All of them
need a check that ensures integer overflow does not oocur.

1. changes to harness.c:
This modification includes a check to ensure that adding ret
to m does not exceed SIZE_MAX.

2. changes to shared.c:
This modification includes a check before the addition to
ensure that (rcvd + ret) or (sent + ret) does not exceed SIZE_MAX.
If the addition would exceed SIZE_MAX, it sets err to an error
value and breaks from the loop, preventing overflow.

Signed-off-by: Juee Himalbhai Desai <juee.himalbhai.desai@intel.com>
@ghost
Copy link

ghost commented Jul 2, 2024

This PR is not needed. This is the case where coverity is not aware of the APIs. We are calculating size used for send/recv calls to not exceed the value (size_t lens) passed to our functions. The for-loop/while-loop will only continue if there is more data. You should disposition the coverity issues as "False Positive" and move on.
Now, I did not check all the loops for correctness, but I do see the intention of the code.

@Juee14Desai
Copy link
Contributor Author

@chien-intel I agree with you. I myself was on the fence but coverity has marked it as a high priority issue and since it falls under CWE top 25 defects, I added this check. If there is a consensus on not needing this, I'll mark it as "False Positive" and close the PR.

@j-xiong Please let me know if you think this is not needed.

@j-xiong
Copy link
Contributor

j-xiong commented Jul 2, 2024

Yes, those are false positives. Coverity doesn't understand the semantics of ofi_send_socket() and ofi_recv_socket() and assumes arbitrary range of return values.

@Juee14Desai
Copy link
Contributor Author

Coverity issues marked "False Positive". Closing the PR.

@Juee14Desai Juee14Desai closed this Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants