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

Check whether a pointer created by dynamic_cast is null before using it. #689

Conversation

pettershao-ragilenetworks
Copy link
Contributor

What I did
Check whether a pointer created by dynamic_cast is null before using it.
Why I did it
dynamic_cast<ConsumerStateTable *>(selectable) may return NULL

@pettershao-ragilenetworks
Copy link
Contributor Author

Hi~ could you please review, thanks. @prsunny @qiluo-msft

@pettershao-ragilenetworks
Copy link
Contributor Author

please review, thanks. @prsunny @qiluo-msft

@@ -233,7 +233,13 @@ void Logger::settingThread()
}

KeyOpFieldsValuesTuple koValues;
dynamic_cast<ConsumerStateTable *>(selectable)->pop(koValues);
Copy link
Contributor

@qiluo-msft qiluo-msft Oct 11, 2022

Choose a reason for hiding this comment

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

selectable

I think it is always of the type ConsumerStateTable? Did you have a use case to trigger a negative case? If yes, could you add a unit test case?

If I understand it correctly, then you can add assert. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expression dynamic_cast T(v) converts the expression v to type T. Type T must be a pointer or reference to a complete class type or a pointer to void. If T is a pointer and the dynamic_cast operator fails, the operator returns a null pointer of type T. If T is a reference and the dynamic_cast operator fails, the operator throws the exception std::bad_cast. You can find this class in the standard library header .

In C++ usage, it would be a problem.
It will not be triggered by the test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally understood what you are saying. My point is that it could not happen due to the function context. Then assert is the recommendation, no need to check in runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, I have changed the error handling.
I think it is necessary to add this judgment to increase system stability.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does add value since the logic will guarantee consumerStateTable != NULL. The extra preventative is okay, then you need to add SWSS_LOG_ERROR to explain, and break instead of continue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed. Please review. thanks.

@pettershao-ragilenetworks
Copy link
Contributor Author

Please review, thanks. @qiluo-msft

qiluo-msft
qiluo-msft previously approved these changes Oct 18, 2022
qiluo-msft
qiluo-msft previously approved these changes Oct 19, 2022
@pettershao-ragilenetworks
Copy link
Contributor Author

Could you please merge, thanks. @qiluo-msft

@qiluo-msft
Copy link
Contributor

Pending on PR checkers passing.

@pettershao-ragilenetworks
Copy link
Contributor Author

I changed file to rerun the failed jobs, could you please review and merge. Thanks! @qiluo-msft

@qiluo-msft qiluo-msft merged commit d0fdf62 into sonic-net:master Oct 24, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Oct 28, 2022
Update sonic-swss-common submodule pointer to include the following:
* abda263 Make the loglevel persistent by moving the LOGGER table from the LOGLEVEL DB to the CONFIG DB ([sonic-net#687](sonic-net/sonic-swss-common#687))
* d0fdf62 Check whether a pointer created by dynamic_cast is null before using it. ([sonic-net#689](sonic-net/sonic-swss-common#689))
* 2cae742 [Fast/Warm restart] Implement helper class for waiting restart done ([sonic-net#691](sonic-net/sonic-swss-common#691))

Signed-off-by: dprital <drorp@nvidia.com>
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