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

C++: Assorted minor doc improvements #16939

Merged
merged 14 commits into from
Jul 11, 2024
Merged

C++: Assorted minor doc improvements #16939

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f64743e
C++: Fix mistake in example for cpp/incorrect-allocation-error-handling.
geoffw0 Jul 4, 2024
4de43e1
C++: Add the examples to the test.
geoffw0 Jul 4, 2024
7abece4
C++: Add a 'good' example for cpp/unsigned-difference-expression-comp…
geoffw0 Jul 4, 2024
1343e4c
C++: Add another 'good' example for cpp/unsigned-difference-expressio…
geoffw0 Jul 4, 2024
5d89872
C++: Add the examples to the test.
geoffw0 Jul 4, 2024
0288499
C++: Rephrase the alert message for cpp/wrong-type-format-argument to…
geoffw0 Jul 4, 2024
3c70583
C++: Add close calls to examples for cpp/toctou-race-condition.
geoffw0 Jul 8, 2024
d52210d
C++: Improve the example for cpp/return-stack-allocated-memory.
geoffw0 Jul 8, 2024
4f0d725
C++: Add a 'good' example as well.
geoffw0 Jul 8, 2024
80af5b7
C++: Add a third example for cpp/world-writable-file-creation.
geoffw0 Jul 8, 2024
8818f63
C++: Add some practical details to the examples.
geoffw0 Jul 8, 2024
7438462
C++: Autoformat.
geoffw0 Jul 10, 2024
0344381
Merge remote-tracking branch 'upstream/main' into docsforautofix
geoffw0 Jul 10, 2024
bf47574
Merge branch 'main' into docsforautofix
geoffw0 Jul 11, 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
1 change: 1 addition & 0 deletions cpp/ql/test/query-tests/Security/CWE/CWE-570/IncorrectAllocationErrorHandling.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@
| test.cpp:151:9:151:24 | new | This allocation cannot throw. $@ is unnecessary. | test.cpp:152:15:152:18 | { ... } | This catch block |
| test.cpp:199:15:199:35 | new | This allocation cannot throw. $@ is unnecessary. | test.cpp:201:16:201:19 | { ... } | This catch block |
| test.cpp:212:14:212:34 | new | This allocation cannot throw. $@ is unnecessary. | test.cpp:213:34:213:36 | { ... } | This catch block |
| test.cpp:246:17:246:31 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:247:8:247:12 | ! ... | This check |
51 changes: 51 additions & 0 deletions cpp/ql/test/query-tests/Security/CWE/CWE-570/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,3 +233,54 @@ void test_operator_new_without_exception_spec() {
int* p = new(42, std::nothrow) int; // GOOD
if(p == nullptr) {}
}

namespace std {
void *memset(void *s, int c, size_t n);
}

// from the qhelp:
namespace qhelp {
// BAD: the allocation will throw an unhandled exception
// instead of returning a null pointer.
void bad1(std::size_t length) noexcept {
int* dest = new int[length];
if(!dest) {
return;
}
std::memset(dest, 0, length);
// ...
}

// BAD: the allocation won't throw an exception, but
// instead return a null pointer. [NOT DETECTED]
void bad2(std::size_t length) noexcept {
try {
int* dest = new(std::nothrow) int[length];
std::memset(dest, 0, length);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example isn't caught because the query assumes that any function with no body may throw:

codeql/cpp/ql/src/Security/CWE/CWE-570/IncorrectAllocationErrorHandling.ql

Line 138 in 6359388

not exists(fc.getTarget()) or

As a follow-up to this PR we could add a whitelist of functions we know don't throw to fix this example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell there is no guarantee that std::memcpy doesn't throw in the language standard, but most implementations won't. The one I'm looking appears to get marked nothrow if it's compiled as C++, but not if it's compiled as C, and it appears to get C linkage and not nothrow in the actual database. It also has a body that just calls a builtin function.

So we could perhaps address this by assuming that a C linkage function doesn't throw ... but I don't think it's actually strictly true that a C linkage function can't throw. Or that builtin function don't throw, but again I'm not convinced that's true. Which leaves us with modelling ... we could model functions that don't throw, but again I don't know if it's strictly true that all versions of memset don't throw, and it seems a bit heavy handed to model it given that nothrow exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

std::memcpy is from the C standard library, so it cannot throw. But you're right that it's not marked as noexcept in the standard.

Copy link
Contributor

@MathiasVP MathiasVP Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

And this is probably the relevant part of the standard:

Screenshot 2024-07-10 at 14 07 22

So since memcpy cannot throw in C, and the contents of cstring and string.h is identical (other than the memchr stuff), my reading is that memcpy also isn't allowed to throw 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You make a good case for modelling. I'll create an issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue created. Thanks for helping me get to the bottom of this!

// ...
} catch(std::bad_alloc&) {
// ...
}
}

// GOOD: the allocation failure is handled appropriately.
void good1(std::size_t length) noexcept {
try {
int* dest = new int[length];
std::memset(dest, 0, length);
// ...
} catch(std::bad_alloc&) {
// ...
}
}

// GOOD: the allocation failure is handled appropriately.
void good2(std::size_t length) noexcept {
int* dest = new(std::nothrow) int[length];
if(!dest) {
return;
}
std::memset(dest, 0, length);
// ...
}
}