Skip to content

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

Merged
qiluo-msft merged 12 commits intosonic-net:masterfrom
pettershao-ragilenetworks:pettershao-ragilenetworks-patch-1
Oct 24, 2022
Merged

Check whether a pointer created by dynamic_cast is null before using it.#689
qiluo-msft merged 12 commits intosonic-net:masterfrom
pettershao-ragilenetworks:pettershao-ragilenetworks-patch-1

Conversation

@pettershao-ragilenetworks
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@pettershao-ragilenetworks
Copy link
Copy Markdown
Contributor Author

please review, thanks. @prsunny @qiluo-msft

Comment thread common/logger.cpp
}

KeyOpFieldsValuesTuple koValues;
dynamic_cast<ConsumerStateTable *>(selectable)->pop(koValues);
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@pettershao-ragilenetworks pettershao-ragilenetworks Oct 11, 2022

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
Copy Markdown
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
Copy Markdown
Contributor Author

@pettershao-ragilenetworks pettershao-ragilenetworks Oct 18, 2022

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Could you please merge, thanks. @qiluo-msft

@qiluo-msft
Copy link
Copy Markdown
Contributor

Pending on PR checkers passing.

@pettershao-ragilenetworks
Copy link
Copy Markdown
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 <[email protected]>
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