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

Jump to bottom

CPP: Add query for CWE-570 detect and handle memory allocation errors. #5010

Merged
merged 14 commits into from Feb 4, 2021
Merged

CPP: Add query for CWE-570 detect and handle memory allocation errors. #5010

merged 14 commits into from Feb 4, 2021

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 as a code owner Jan 24, 2021
@ihsinme
Copy link
Contributor Author

@ihsinme ihsinme commented Jan 24, 2021

Information about the found and accepted fix in the project:

google/sentencepiece#581
assimp/assimp#3569

geoffw0
geoffw0 reviewed Jan 26, 2021
View changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

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. :)

.../src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.cpp Show resolved Hide resolved
...rc/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.qhelp Outdated Show resolved Hide resolved
...rc/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.qhelp Outdated Show resolved Hide resolved
...rc/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.qhelp Outdated Show resolved Hide resolved
...rc/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.qhelp Outdated Show resolved Hide resolved
...l/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql Outdated Show resolved Hide resolved
...l/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql Show resolved Hide resolved
...l/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql Outdated Show resolved Hide resolved
...l/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql Show resolved Hide resolved
...l/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql Show resolved Hide resolved
@ihsinme
Copy link
Contributor Author

@ihsinme 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 ihsinme commented Jan 29, 2021

I would like to hear your opinion.

@ihsinme ihsinme mentioned this pull request Jan 29, 2021
Closed
1 task
ihsinme and others added 2 commits Jan 29, 2021
@ihsinme @geoffw0
Apply suggestions from code review
bdbf5a4 
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
@ihsinme
Update WrongInDetectingAndHandlingMemoryAllocationErrors.ql
2b946ae
geoffw0
geoffw0 previously approved these changes Feb 2, 2021
View changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

LGTM, the CI tests are now running...

@geoffw0
Copy link
Contributor

@geoffw0 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;.

geoffw0
geoffw0 approved these changes Feb 4, 2021
View changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Great, merging...

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

@geoffw0 geoffw0

Assignees
No one assigned
Labels
C++ documentation
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
2 participants
@ihsinme @geoffw0