Skip to content

C++: Fix FPs in cpp/badly-bounded-write caused by extraction errors #18222

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

Merged
jketema merged 5 commits into from
Dec 6, 2024
Merged

C++: Fix FPs in cpp/badly-bounded-write caused by extraction errors #18222

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd74698
C++: Test case for cpp/badly-bounded-write
calumgrant Dec 5, 2024
b7f47f7
C++: Remove FPs from cpp/badly-bounded-write
calumgrant Dec 5, 2024
7aed4c3
C++: Change note
calumgrant Dec 5, 2024
d38975b
C++: Use getType() instead of getUnderlyingType()
calumgrant Dec 6, 2024
e98129c
Update cpp/ql/src/change-notes/2024-12-05-badly-bounded-write.md
calumgrant Dec 6, 2024
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
3 changes: 2 additions & 1 deletion cpp/ql/src/Security/CWE/CWE-120/BadlyBoundedWrite.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ from BufferWrite bw, int destSize
where
bw.hasExplicitLimit() and // has an explicit size limit
destSize = max(getBufferSize(bw.getDest(), _)) and
bw.getExplicitLimit() > destSize // but it's larger than the destination
bw.getExplicitLimit() > destSize and // but it's larger than the destination
not bw.getDest().getUnderlyingType().stripType() instanceof ErroneousType // destSize may be incorrect
Copy link
Contributor

@jketema jketema Dec 6, 2024

Choose a reason for hiding this comment

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

Yes, I believe stripType takes care of that already.

Copy link
Contributor Author

@calumgrant calumgrant Dec 6, 2024

Choose a reason for hiding this comment

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

I got confused, stripType() is only on Type.

Copy link
Contributor

@jketema jketema Dec 6, 2024

Choose a reason for hiding this comment

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

What's the type of getDest?

Copy link
Contributor Author

@calumgrant calumgrant Dec 6, 2024

Choose a reason for hiding this comment

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

It is Expr.

Copy link
Contributor

@jketema jketema Dec 6, 2024

Choose a reason for hiding this comment

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

You can probably just change this to getType().stripType() in that case.

select bw,
"This '" + bw.getBWDesc() + "' operation is limited to " + bw.getExplicitLimit() +
" bytes but the destination is only " + destSize + " bytes."
4 changes: 4 additions & 0 deletions cpp/ql/src/change-notes/2024-12-05-badly-bounded-write.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The "Badly bounded write" query (`cpp/badly-bounded-write`) query no longer produces results if there is an extraction error in the type of the output buffer.
11 changes: 11 additions & 0 deletions cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/errors.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// semmle-extractor-options: --expect_errors

typedef unsigned long size_t;
typedef int wchar_t;

int swprintf(wchar_t *s, size_t n, const wchar_t *format, ...);

void test_extraction_errors() {
WCHAR buffer[3];
swprintf(buffer, 3, L"abc");
}
Loading