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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Oct 24, 2022
Merged

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

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f437c47
Update logger.cpp
pettershao-ragilenetworks Sep 15, 2022
222d457
Update logger.cpp
pettershao-ragilenetworks Sep 16, 2022
599300e
Update logger.cpp
pettershao-ragilenetworks Sep 16, 2022
e24f375
Update logger.cpp
pettershao-ragilenetworks Sep 16, 2022
f89ff33
Update logger.cpp
pettershao-ragilenetworks Sep 20, 2022
dca8229
Update logger.cpp
pettershao-ragilenetworks Sep 21, 2022
a9e7bba
Update logger.cpp
pettershao-ragilenetworks Oct 11, 2022
1130c0b
Update logger.cpp
pettershao-ragilenetworks Oct 12, 2022
d697a02
Update logger.cpp
pettershao-ragilenetworks Oct 18, 2022
9768308
Update logger.cpp
pettershao-ragilenetworks Oct 19, 2022
022ff37
Update logger.cpp
pettershao-ragilenetworks Oct 19, 2022
bb43f05
rerun failed job
pettershao-ragilenetworks Oct 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion common/logger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,14 @@ void Logger::settingThread()
}

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

@qiluo-msft qiluo-msft Oct 11, 2022 •
edited
Loading

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.

ConsumerStateTable *consumerStateTable = NULL;
consumerStateTable = dynamic_cast<ConsumerStateTable *>(selectable);
if (consumerStateTable == NULL)
{
SWSS_LOG_ERROR("dynamic_cast returned NULL");
break;
}
consumerStateTable->pop(koValues);
std::string key = kfvKey(koValues), op = kfvOp(koValues);

if (op != SET_COMMAND || !m_settingChangeObservers.contains(key))
Expand Down