Skip to content

Conversation

@ihsinme
Copy link
Contributor

@ihsinme ihsinme commented Jan 24, 2021

In this query I am trying to find situations of incorrect handling of memory allocation using the new operator.
This error is quite common in projects and can lead to a violation of the logic of the program or to an unhandled crash.

of course, we can consider any selection without processing to be incorrect, but in this request I am considering exactly the situation of confusion. when the developer confused what kind of processing to apply. this allows us to understand that he tried to handle the case when the memory will not be allocated, but did not handle it correctly.

this is my first test file in C ++, it turned out to be rather weak, in the future I will think about improving it.

@ihsinme ihsinme requested a review from a team as a code owner January 24, 2021 21:12
@ihsinme
Copy link
Contributor Author

ihsinme commented Jan 24, 2021

Information about the found and accepted fix in the project:

google/sentencepiece#581
assimp/assimp#3569

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Hi, this looks like an interesting query for spotting simple errors in the use of new. As always I appreciate that you have included tests and qhelp. I think the query logic could be cleaner in a few places, but the comments make it fairly clear what is going on.

I intend to try this query out on LGTM and see what kinds of results we get. Based on what you're trying to do, I hope the results will be quite good. :)

@ihsinme
Copy link
Contributor Author

ihsinme commented Jan 26, 2021

thanks for your comments.
I ask you to give me the opportunity to correct these comments on my own, even if it will take more time, but this will allow better writing of the following queries.

@ihsinme
Copy link
Contributor Author

ihsinme commented Jan 29, 2021

I would like to hear your opinion.

ihsinme and others added 2 commits January 29, 2021 13:41
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
geoffw0
geoffw0 previously approved these changes Feb 2, 2021
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM, the CI tests are now running...

@geoffw0
Copy link
Contributor

geoffw0 commented Feb 4, 2021

The checks have raised a couple of issues:

ql/cpp/ql/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql would change by autoformatting.

i.e. the query file needs autoformatting.

ql/cpp/ql/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.qhelp:6:269: The entity name must immediately follow the '&' in the entity reference.

I think the & should be escaped as &amp;.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Great, merging...

@geoffw0 geoffw0 merged commit 7c54512 into github:main Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants