Skip to content

Klocwork check null before dereference in acl_profiler.cpp #203

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

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

haoxian2
Copy link
Contributor

@haoxian2 haoxian2 commented Nov 17, 2022

Fixed the following Klocwork issue:

  1. Pointer 'accel_def' returned from call to function 'acl_find_accel_def' at line 605 may be NULL and may be dereferenced at line 620.

acl_find_accl_def bails out if the return value would be NULL, therefore ensuring that accel_def can never be NULL if status == CL_SUCCESS. So an assert statement is included instead after the status check, which would rule out NULL return values.

Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @haoxian2! This one needs a bit more investigation as detailed below.

@haoxian2 haoxian2 force-pushed the klocwork-acl_profiler branch 2 times, most recently from 03f4bb8 to ccf7254 Compare November 18, 2022 18:29
@haoxian2 haoxian2 requested a review from pcolberg November 18, 2022 18:31
@pcolberg pcolberg added the bug Something isn't working label Nov 18, 2022
@pcolberg pcolberg added this to the 2023.1 milestone Nov 18, 2022
@haoxian2
Copy link
Contributor Author

accel_def receives the return value of acl_find_accl_def, but acl_find_accl_def also sets the status variable. There's a line in acl_find_accl_def that specifically says if the return value would be NULL, then status would be set to CL_INVALID_KERNEL_NAME and it would return 0. Therefore, the subsequent status check would not pass if acl_find_accl_def would return a NULL. The assert inserted would only be reached if status == CL_SUCCESS.

acl_find_accel_def ensures that the returned value is not NULL.
@pcolberg
Copy link
Contributor

accel_def receives the return value of acl_find_accl_def, but acl_find_accl_def also sets the status variable. There's a line in acl_find_accl_def that specifically says if the return value would be NULL, then status would be set to CL_INVALID_KERNEL_NAME and it would return 0. Therefore, the subsequent status check would not pass if acl_find_accl_def would return a NULL. The assert inserted would only be reached if status == CL_SUCCESS.

Thanks @haoxian2 for the detailed explanation and my apologies for missing this the first time, which you had already confirmed in the commit message.

@pcolberg pcolberg force-pushed the klocwork-acl_profiler branch from ccf7254 to d564f2a Compare November 23, 2022 20:06
Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @haoxian2. I pushed a minor revision of the commit message, adding a tag in the subject and moving the reasoning to the body to keep the subject brief.

@pcolberg pcolberg merged commit 4401942 into intel:main Nov 23, 2022
@haoxian2 haoxian2 deleted the klocwork-acl_profiler branch November 23, 2022 20:45
@haoxian2
Copy link
Contributor Author

Thanks @pcolberg for the revision!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants