Skip to content

check for null dereference in trackingOpenIpcHandle #419

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

Closed
wants to merge 1 commit into from

Conversation

bratpiorka
Copy link
Contributor

@bratpiorka bratpiorka commented Apr 8, 2024

Description

Check for null dereference in trackingOpenIpcHandle - this is a fix for coverity issue https://scan3.scan.coverity.com/#/project-view/65229/15141?selectedIssue=453142

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • Logger (with debug/info/... messages) is used

@@ -586,17 +586,23 @@ static umf_result_t trackingOpenIpcHandle(void *provider, void *providerIpcData,
(umf_tracking_memory_provider_t *)provider;
umf_result_t ret = UMF_RESULT_SUCCESS;

if (p->hUpstream == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How it is possible? If it happens it indicates error in UMF code, right? If so, I think it should be just assert.

I agree that && p->hUpstream checks below should be just removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a fix for coverity. Agree that this should never happen but I think it is better to add this code that add a false-positive

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO if it is really a false-positive it is much better to ignore it in Coverity than add a redundant code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bratpiorka I propose sth like that #420 instead and ignore this issue in Coverity.

@bratpiorka bratpiorka marked this pull request as ready for review April 9, 2024 07:35
@bratpiorka bratpiorka requested a review from a team as a code owner April 9, 2024 07:35
@bratpiorka
Copy link
Contributor Author

this would be fixed by #420

@bratpiorka bratpiorka closed this Apr 9, 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.

3 participants