-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
C++: Assorted minor doc improvements #16939
Conversation
… be less prescriptive.
QHelp previews: cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.qhelpReturning stack-allocated memoryThis rule finds return statements that return pointers to an object allocated on the stack. The lifetime of a stack allocated memory location only lasts until the function returns, and the contents of that memory become undefined after that. Clearly, using a pointer to stack memory after the function has already returned will have undefined results. RecommendationUse the functions of the ExampleThe following example allocates an object on the stack and returns a pointer to it. This is incorrect because the object is deallocated when the function returns, and the pointer becomes invalid. Record *mkRecord(int value) {
Record myRecord(value);
return &myRecord; // BAD: returns a pointer to `myRecord`, which is a stack-allocated object.
}
To fix this, allocate the object on the heap using Record *mkRecord(int value) {
Record *myRecord = new Record(value);
return myRecord; // GOOD: returns a pointer to a `myRecord`, which is a heap-allocated object.
}
References
cpp/ql/src/Security/CWE/CWE-732/DoNotCreateWorldWritable.qhelpFile created without restricting permissionsWhen you create a file, take care to give it the most restrictive permissions possible. A typical mistake is to create the file with world-writable permissions. This can allow an attacker to write to the file, which can give them unexpected control over the program. RecommendationFiles should usually be created with write permissions only for the current user. If broader permissions are needed, including the users' group should be sufficient. It is very rare that a file needs to be world-writable, and care should be taken not to make assumptions about the contents of any such file. On Unix systems, it is possible for the user who runs the program to restrict file creation permissions using ExampleThis example shows two ways of writing a default configuration file. Software often does this to provide the user with a convenient starting point for defining their own configuration. However, configuration files can also control important aspects of the software's behavior, so it is important that they cannot be controlled by an attacker. The first example creates the default configuration file with the usual "default" Unix permissions, void write_default_config_bad() {
// BAD - this is world-writable so any user can overwrite the config
int out = creat(OUTFILE, 0666);
if (out < 0) {
// handle error
}
dprintf(out, "%s", DEFAULT_CONFIG);
close(out);
}
void write_default_config_good() {
// GOOD - this allows only the current user to modify the file
int out = creat(OUTFILE, S_IWUSR | S_IRUSR);
if (out < 0) {
// handle error
}
dprintf(out, "%s", DEFAULT_CONFIG);
close(out);
}
void write_default_config_good_2() {
// GOOD - this allows only the current user to modify the file
int out = open(OUTFILE, O_WRONLY | O_CREAT, S_IWUSR | S_IRUSR);
if (out < 0) {
// handle error
}
FILE *fd = fdopen(out, "w");
if (fd == NULL) {
close(out);
// handle error
}
fprintf(fd, "%s", DEFAULT_CONFIG);
fclose(fd);
} References
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! Now we just need to make CI happy 😄
void bad2(std::size_t length) noexcept { | ||
try { | ||
int* dest = new(std::nothrow) int[length]; | ||
std::memset(dest, 0, length); |
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
The integration test is still failing, and I don't think it's anything to do with the PR. So I'll try merging in |
The integration tests are failing because you changed the alert message of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good—thank you!
Ah, I didn't notice that. I've made a semmle-code PR referencing this one, that fixes up the integration test. The tests should pass there. |
Integration tests pass on the internal PR. I'll merge both together once both are approved. |
Thanks for merging when all was green. :) |
No problem! 😄 |
Assorted minor doc improvements. Most of the changes are intended to provide better context for autofix (e.g. clearer examples), or just to correct mistakes noticed while working on triage. These changes should benefit human readers as well as autofix.
I'm happy to explain the thinking behind any particular change where it is not clear.