github / codeql-coding-standards Goto Github PK
View Code? Open in Web Editor NEWThis repository contains CodeQL queries and libraries which support various Coding Standards.
License: MIT License
This repository contains CodeQL queries and libraries which support various Coding Standards.
License: MIT License
A0-1-6
Alias templates (i.e. type aliases which are templated) are always identified as unused type declarations under A0-1-6
.
template <class T> class Y {}; // COMPLIANT - used in test case below
// Alias template
template <typename T> using Z = Y<T>; // COMPLIANT - used below
void test_alias_template() {
Z<int> v;
}
Currently we have a library for Cpp14 specific naming, but would benefit from creating a set of C99 specific names to avoid any false positives of standard expansions that occurred. Ideally this extraction could be automated and some script to accomplish that would also be contributed to the repo. This requires us to first identify a good source to extract this information from.
A false positive alert is reported for the following code example which is not trying to implement a bitwise or arithmetic operator:
User-defined bitwise or arithmetic operator test::operator<<(ostream &, const Test &) -> ostream & does not return a prvalue.
#include <ostream>
namespace test {
struct Test {};
std::ostream& operator<<(std::ostream& os, const Test&) { // A13-2-2 reported here
os << "test";
return os;
}
} // namespace test
A3-9-1
This rule is intended to prevent the use of "basic numerical types". It cites char
as one such type, however char
is an interesting case because it's intended primarily to store character data not numeric data. The signed char
and unsigned char
types, which are distinct, are intended to represent numeric data, and, unlike char
, are part of the standard integer types.
We should consider either:
char
case into a new query, so that users can apply a deviation.char
s, and adding an "Implementation scope" note to specify that we do not enforce this part of the standard.char x; // compliant? separate rule?
signed char x; // NON_COMPLIANT
unsigned char x; // NON_COMPLIANT
A15-4-2
Copy elision allows the compiler to omit copy and move construction in certain circumstances. Some of these circumstances are considered "mandatory" - i.e. the compiler must omit the explicit construction - and in those circumstances copy and move constructors may never be called.
If you have move/copy constructors which are never expected to be called because they are always elided, you may decide to throw an exception from the copy/move constructor to enforce that it is only used in circumstances where the elision occurs. As our call graph does not currently reflect the mandatory elision of functions, this can cause false positives.
// `foo` is only used in circumstances where it is elided, so this exception is never thrown in practice
constexpr foo(const foo&) : dummy() {
throw some_bad_exception();
};
A2-10-1
A2-10-1
states "An identifier declared in an inner scope shall not hide an identifier declared in an outer scope.". The existing query looks for user defined variables which cause hiding, but functions and types can also be hidden (and cause hiding).
int a;
namespace b {
void a() { } // NON_COMPLIANT
int c() { return a; }
}
A18-5-8
Copy elision allows a call to a copy or move constructor to be omitted in certain cases where it is safe to do so ([class.copy]/31
), such as copying/moving from a temporary object or where a copy/move occurs as part of a return from a function and the source is an local scope variable within that function.
Where copy elision applies, the CodeQL C++ extractor appears to remove the implicit copy/move calls. This is problematic because the query for this rule uses the presence of a copy/move constructor to determine whether an object outlives the lifetime of the function, thus causing false positives.
Consider the following example:
std::unique_ptr<C1> Create(const std::string& s) noexcept {
return std::make_unique<C1>(s);
}
Without copy elision, we would assume an implicit move/copy constructor call would exist here. However, due to copy elision the constructor call does not exist.
MakeSharedOrUnique.isAlwaysFreed()
should be updated to consider the local data flow to the expression of a return statement as evidence that the heap memory associated with the shared or unique pointer is not freed within this function.
The test here:
https://github.com/github/codeql-coding-standards/blob/main/cpp/autosar/test/rules/M2-13-3/test.cpp
Is overly simple and fails to test for octal and binary literals, or literals with a type suffix such a L or LL. Notice also that the non-decimal integer literals will switch between signed and unsigned types as their size increases, so there are several different ranges of unsigned values.
void test_unsigned_literals_without_suffix() {
0xFFFFFFFFU; // COMPLIANT - literal explicitly marked as unsigned
0xFFFFFFFF; // NON_COMPLIANT - literal is too large for a signed int, so has
// type unsigned int
}
Add some more tests:
Here is a an example that includes code to print the type of the integer literals:
#include <iostream>
#include <string_view>
template <typename T>
constexpr auto type_name() {
std::string_view name, prefix, suffix;
#ifdef __clang__
name = __PRETTY_FUNCTION__;
prefix = "auto type_name() [T = ";
suffix = "]";
#elif defined(__GNUC__)
name = __PRETTY_FUNCTION__;
prefix = "constexpr auto type_name() [with T = ";
suffix = "]";
#elif defined(_MSC_VER)
name = __FUNCSIG__;
prefix = "auto __cdecl type_name<";
suffix = ">(void)";
#endif
name.remove_prefix(prefix.size());
name.remove_suffix(suffix.size());
return name;
}
int main() {
auto v1 = 2'147'483'647;
std::cout << "decltype(v1) is " << type_name<decltype(v1)>() << '\n';
auto v2 = 2'147'483'648;
std::cout << "decltype(v2) is " << type_name<decltype(v2)>() << '\n';
auto v3 = 32'768L;
std::cout << "decltype(v3) is " << type_name<decltype(v3)>() << '\n';
auto v4a = 0x7FFFFFFF;
std::cout << "decltype(v4a) is " << type_name<decltype(v4a)>() << '\n';
auto v4b = 0xFFFFFFFF; // NON-COMPLIANT
std::cout << "decltype(v4b) is " << type_name<decltype(v4b)>() << '\n';
auto v4b1 = 0x1FFFFFFFF; // NON-COMPLIANT - can't be represented as a int
std::cout << "decltype(v4b1) is " << type_name<decltype(v4b1)>() << '\n';
auto v4c = 0x7FFFFFFFFFFFFFFF;
std::cout << "decltype(v4c) is " << type_name<decltype(v4c)>() << '\n';
auto v4d = 0xFFFFFFFFFFFFFFFF;
std::cout << "decltype(v4d) is " << type_name<decltype(v4d)>() << '\n';
/*
auto v4e = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
std::cout << "decltype(v4e) is " << type_name<decltype(v4e)>() << '\n';
auto v4f = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
std::cout << "decltype(v4f) is " << type_name<decltype(v4f)>() << '\n';
*/
auto v5 = -2'147'483'648;
std::cout << "decltype(v5) is " << type_name<decltype(v5)>() << '\n';
auto v5p = -v5;
std::cout << "decltype(v5p) is " << type_name<decltype(v5p)>() << '\n';
auto v5q = -2'147'483'647 - 1;
std::cout << "decltype(v5q) is " << type_name<decltype(v5q)>() << '\n';
auto v5r = (int)-2'147'483'648;
std::cout << "decltype(v5r) is " << type_name<decltype(v5r)>() << '\n';
auto v6 = 037'777'777'777; // NON-COMPLIANT
std::cout << "decltype(v6) is " << type_name<decltype(v6)>() << '\n';
auto v7 = 017'777'777'777;
std::cout << "decltype(v7) is " << type_name<decltype(v7)>() << '\n';
auto v8 = 0b1111'1111'1111'1111'1111'1111'1111'1111; // Non-compliant
std::cout << "decltype(v8) is " << type_name<decltype(v8)>() << '\n';
auto v9 = 0b0111'1111'1111'1111'1111'1111'1111'1111;
std::cout << "decltype(v9) is " << type_name<decltype(v9)>() << '\n';
}
A15-4-2
ERR55-CPP
noexcept
functions frequently call other noexcept
functions. If a noexcept
function throws an exception, then we currently report that as a violation in every noexcept
function that calls that function. This creates multiple alerts for one issues. We should instead report the original instance and not the others.
void f1() noexcept {
throw foo(); // report this case
}
void f2() noexcept {
f1(); // do not report this case
}
M0-2-1
The query currently identifies objects using an intra-procedural technique - we only check for equivalence of objects within the same function. We should consider expanding to support analysis across functions.
struct s1 {
int m1[10];
};
struct s2 {
int m1;
struct s1 m2;
};
union u {
struct s1 m1;
struct s2 m2;
};
void overlapping_access(u u1, u u2) {
u1.m2.m2 = u2.m1; // NON_COMPLIANT when called from test
}
void test() {
u1 u;
overlapping_access(u, u)
}
M27-0-1
M27-0-1
prohibits the use of the stream input/output library <cstdio>
. However, we currently flag uses of size_t
as from cstdio
, which is undesirable (as it can be provided by any of a number of headers, and is not strictly part of the stream input/output library).
Any use of size_t
.
A false positive has been reported for the example below.
Additionally:
class A8_4_7 {
public:
std::array<char, 20UL> values;
};
void output(const A8_4_7 &a847) noexcept { // trivial to copy but size is larger than 2 words. (false positive)
std::cout << a847.values[0] << std::endl;
}```
A0-1-3
Missing the following function use cases:
[[maybe_unused]]
=delete
Describe the bug
Implementation of Rule A1-1-1 only looks for deprecated features and not use of implementation extensions
Expected behavior
// __try // Non-compliant - __try is a part of Visual Studio extension
try // Compliant - try keyword is a part of C++ Language Standard
Additional context
https://github.com/github/codeql-coding-standards/tree/main/cpp/autosar/src/rules/A1-1-1
The rule looks at expression passed to decltype
that exhibit side effects.
The goal of this is to detect cases where a programmer relies on side effects that do not occur because the decltype
operand is not evaluated.
However, this cannot be the case for dectype
usage in function or method signatures, but we still report these.
auto foo(...) -> decltype(<expr with side effects>)
}
Exclude decltype
part of a function/method declaration.
You should add a test for MO-1-2 for a infinite loop that breaks after testing a volatile-qualified object, even if that object is not modified within the loop. This loop should be considered compliant.
DCL51-CPP
The rule currently enforces that function names defined in standard library headers are not reused in any namespace. However, a careful re-reading of the C++ standard suggests that's overly specific. [reserved.names]
specifically states that the only kinds of name that are reserved are macros, "global names" and "names with external linkage". In [extern.names]
, the standard says:
Each global function signature declared with external linkage in a header is reserved to the implementation to designate that function signature with external linkage.
So only global function signatures are reserved, and only where they have external linkage. I think we need to do the following:
We may also need to review the rules for objects and _
prefixes.
namespace MyNamespace {
void all_of(); // COMPLIANT
}
AUTOSAR 19-03 rule A7-1-8 - “A non-type specifier shall be placed before a type specifier in a declaration” is missing and has not been implemented.
M0-2-1
The query as currently written only considers overlapping as caused by unions. We should also consider whether overlapping arrays are covered by the same rule.
#include <cstring>
int16_t a[20];
void f2(void) {
std::memcpy(&a[0], &a[1], 10u * sizeof(a[0])); // Non-compliant
std::memmove(&a[0], &a[1], 10u * sizeof(a[0])); // Compliant
std::memcpy(&a[1], &a[0], 10u * sizeof(a[0])); // Non-compliant
std::memmove(&a[1], &a[0], 10u * sizeof(a[0])); // Compliant
}
False positive reported for the following test case. In the example, below, a712
must be constexpr
for value
to be declared constexpr
. However, it is not possible to declare parameters (easily) as constexpr
. The mitigation for this issue is likely that cases where it is not possible to declare the qualifier as constexpr
.
class A7_1_2 {
public:
constexpr auto GetValue() const noexcept { return value_; }
void SetValue(int32_t value) noexcept { value_ = value; }
private:
int32_t value_;
};
std::ostream &operator<<(std::ostream &os, const A7_1_2 a712) noexcept {
const auto value = a712.GetValue(); // Variable value could be marked 'constexpr'.
os << value;
return os;
}
The rule warns about the vector container which is resized after delectation.
Access of container of type Payload does not ensure that the index is smaller than the bounds.
#include <iostream>
#include <string>
#include <vector>
#include <filesystem>
using namespace std;
namespace fs = std::filesystem;
int main() {
typedef vector<uint8_t> Payload;
wstring file(L"This is a wstring");
uint64_t attributes;
Payload serialized_data; serialized_data.resize(file.size() * sizeof(wchar_t) + sizeof(attributes));
*(uint64_t*)&serialized_data.front() = attributes;
/*
* Append the path.
*/
file.copy((wchar_t*)&serialized_data[sizeof(attributes)], file.size());
return 0;
}
M5-3-1
A5-0-2
cpp/autosar/non-boolean-if-condition
cpp/autosar/non-boolean-iteration-condition
Iteration or if statements of type bool
within uninstantiated templates have an unknown
type and raise a false positive.
template<T>
void foo(std::vector<T> & v) {
for (typename std::vector<T>::iterator it{v.begin()}; it != v.end(); ++it) {
(*it)++;
}
}
M0-1-9
This rule produces false-positives for the bool-constexpr
of every declaration of static_assert
. We can probably resolve this case by excluding all Expr
that are within StaticAssert::getCondition().
I have this code snippet
#include <iostream>
#include <string>
#include <set>
#include <map>
int main() {
std::set<std::string> std_section_names;
std_section_names.insert(".text");
std_section_names.insert(".itext");
// ----
std::map<std::string, double> pe_attr = {
{"instructionsCount", 0.0},
};
return 0;
}
Where I created a set
of strings
and a map
with string
to double
in order to count frequency. However, the CTR57-CPP
hits with this message
Comparator 'std::less<basic_string<char, char_traits<char>, allocator<char>>>' used on container or sorting algorithm that is not strictly weakly ordered
```.
I understand the issue very well, yet I don't know how to fix it properly.
code-identifier
for deviations.Describe the bug
When running the analysis_report.py
script the following error is observed:
ImportError: cannot import name 'CodeQL' from partially initialized module 'codeql' (most likely due to a circular import)
Environment
A0-1-2
Consider:
void
.std::ignore
std::ignore = f();
A3-3-1
Add additional compliant declarations to the test code.
The test file test.cpp is missing some tests.
// I would add the following lines to the text case
const int c = 1; // COMPLIANT - internal linkage
const char* const str2 = "foo"; // COMPLIANT - internal linkage
constexpr int k = 1; // COMPLIANT - internal linkage
Most const-qualified objects have internal linkage.
See the [basic.link] subclause of the C++ Standard for additional information.
M0-1-4
constexpr
variables used as a type argument are not recognised as a use of that variable, and therefore cause false positives. This is because constant expressions used as type arguments are stored in the database fully evaluated. Access of constant variable is therefore replaced with the constant value.
This may require a CodeQL extractor change if no use information is currently stored in the database.
In this example:
class C1 {
static constexpr std::uint8_t size{2}; // COMPLIANT[FALSE_POSITIVE]
std::array<bool, size> array{ false, false }; // size is used here
};
size
is used as a template argument for std::array
. However, the type represented in the database is array<bool, 2UL>
, because
A2-5-2
The checker for "Rule A2-5-2 (required, implementation, automated) Digraphs shall not be used." was not implemented. Presumably, the expectation was that compiler flags would be sufficient. However, this is not the case.
Clang has the following flag:
-fno-digraphs
Disables alternative token representations <:
, :>
, <%
, %>
, %:
, %:%:
(default)
I haven't tested this, but presumably disable means that it stops converting them into the corresponding characters. However, to be compliant with the rule we need to diagnose these sequences of characters, even if they are not translated.
GCC is much worse, as they have no checker at all so there is no way to enforce this rule if you are using this compiler.
Context: #45 (comment)
Although this query will catch a lot real-world examples of flawed usage of weak cmpxchg functions (e.g., those used in one-off if-statements), I think that if false-positives don't become an issue, it can be improved by verifying that either:
The loop is infinite or
The loop condition contains the call and checks its result or
The loop condition checks a variable which is, within the loop, assigned either of the following:
The return value of the weak cmpxchg function call (via local data flow)
A literal assigned in and only in a block guarded by a success condition of the weak cmpxchg function call return value
or
To avoid false positives, any value assigned in a block guarded by the weak cmpxchg function call return value
The rule hits on a fixed string length.
The provided code snippet shows a simple 16-base converter yet the rule states
Access of container of type const string does not ensure that the index is smaller than the bounds.
Even though the reminder operation minimum value will be always 0
and the string d
will never be empty.
#include <iostream>
#include <string>
int main() {
std::string word(" ");
auto num = 100000;
static const std::string d = "0123456789ABCDEF";
while (num > 0) {
word = d[num % 16] + word;
num /= 16;
}
return 0;
}
solve issue discussed here
A0-1-4
Add or validate that the following are considered as uses of parameters:
[[maybe_unused]]
std::ignore
.void f(
int i, // compliant
int j, // compliant
int k // compliant
) {
static_cast<void>(i);
(void)j;
std::ignore = k;
}
A12-0-1
Class templates raise false positives even if all special member functions are defined. Identical classes (which are not class templates) do not result in a false-positive result.
template<typename T>
class A12_0_1a { // <= A12-0-1 error here
public:
explicit A12_0_1a(T i) : i_(i) {}
A12_0_1a(const A12_0_1a&) = delete;
A12_0_1a(A12_0_1a&&) = delete;
A12_0_1a& operator=(const A12_0_1a&) = delete;
A12_0_1a& operator=(A12_0_1a&&) = delete;
virtual ~A12_0_1a() = default;
private:
T i_;
};
class A12_0_1b { // <= No A12-0-1 error here
public:
explicit A12_0_1b(int i) : i_(i) {}
A12_0_1b(const A12_0_1b&) = delete;
A12_0_1b(A12_0_1b&&) = delete;
A12_0_1b& operator=(const A12_0_1b&) = delete;
A12_0_1b& operator=(A12_0_1b&&) = delete;
virtual ~A12_0_1b() = default;
private:
int i_;
};
In the following code the member variable t_
is modified by the non-const member function Init
. If the class is not a template no error is reported for similar code.
template<typename T>
class A7_1_1b final {
public:
explicit A7_1_1b(std::int64_t i) noexcept { t_.Init(i); } // t_ is modified here by Init
private:
T t_; // <= A7-1-1: Non-constant variable t_ is used for an object and is not modified.
};
#include <iostream>
#include <vector>
namespace test {
/// doc
template <class T>
class MyClass final {
private:
// CodeQL reports: A7-1-1:cpp/autosar/declaration-unmodified-object-missing-const-specifier
// Non-constant variable data_ is used for an object and is not modified.
// Marking this as "const" is incorrect, we can modify the data through the iterator returned
// by the begin() function, adding const here does not compile.
std::vector<T> data_;
};
} // namespace test
/// main
int main(int, char**) noexcept {
try {
test::MyClass<int> c({1, 2, 3});
} catch (const std::exception&) {
}
}
There should be a test where the side-effecting and no-side effecting functions are in a different translation unit:
What is the expected behavior in such as case?
A2-10-5
A2-10-5
requires that non-member objects are given a unique name (irrespective of scope). However, the query does not currently account for variable templates, which are represented as multiple different variables in the CodeQL C/C++ model.
I think the most straightforward fix here is to exclude the variable template instantiations, and only use the un-instantiated template, e.g.:
not o1.isFromTemplateInstantiation(_) and
not o2.isFromTemplateInstantiation(_)
template<class T> constexpr T number_one = T(1);
int test() {
return number_one<int>;
}
long test2() {
return number_one<long>;
}
Support coding-standards.yaml
in addition to coding-standards.yml
.
M5-0-2
Our current query identifies gratuitous parentheses i.e. parentheses that are not strictly required. However, the rule is actually "Limited dependence should be placed on C++ operator precedence rules in expressions." In other words, we should be identifying cases where a lack of parentheses causes ambiguity.
A15-2-2
A15-2-2 customizes the exception flow model to represent that new
calls may throw a std::bad_alloc
exception. This code correctly handles the case that a noexcept
constructor is called, but should be expanded to include a test case.
The comments for this test all indicate the first three data structures are classes, while they are clearly structs.
A18-0-1
The query for this rule reports any use of headers with file names the same as a prohibited C standard library header. This can cause false positives if the included file is not from a C standard library implementation but just happens to have the same name as a C standard library header.
There's no certain way to determine whether an include is of a C Standard Library header file, because the files themselves are not universally distinguishable, so we will need to consider some heuristics for identification.
As an initial idea, we could only report cases where:
#include
specifies no file path (e.g. filename = i.getIncludeText().substring(1, i.getIncludeText().length() - 2)
)not exists(i.getIncludedFile().getRelativePath())
$ cat lib/example.h
#ifndef LIB_EXAMPLE_H_
#define LIB_EXAMPLE_H_
#endif
$ cat test.cpp
#include "lib/example.h" // A18-0-1 reported here
#include <iostream>
STR32-C
Support reducing buffer size via realloc.
#include <stdlib.h>
#include <wchar.h>
wchar_t *cur_msg = NULL;
size_t cur_msg_size = 1024;
size_t cur_msg_len = 0;
void lessen_memory_usage(void) {
wchar_t *temp;
size_t temp_size;
/* ... */
if (cur_msg != NULL) {
temp_size = cur_msg_size / 2 + 1;
temp = realloc(cur_msg, temp_size * sizeof(wchar_t));
/* temp &and cur_msg may no longer be null-terminated */
if (temp == NULL) {
/* Handle error */
}
cur_msg = temp;
cur_msg_size = temp_size;
cur_msg_len = wcslen(cur_msg);
}
}
The user manual describes how to use the CLI to run the coding standards queries.
However, it would be useful to add a how to get started section to the README that describes how to use the coding standards queries in a GitHub Action workflow.
M3-2-1
In CodeQL variable templates are represented by multiple different variables (an uninstantiated copy, plus a copy for each instantiation). As each of these can have a different type, they currently cause false positives for M3-2-1
, which is about finding redeclarations of the same object with incompatible types.
The easiest way to exclude templates is to remove versions from template instantiations:
not decl1.isFromTemplateInstantiation(_) and
not decl2.isFromTemplateInstantiation(_)
While we are making modifications to this query, there are a few other changes we should make:
select decl1, "The object $@ of type $@ is not compatible with re-declaration $@ of type $@", decl1,
decl1.getName(), decl1.getType(), decl1.getType().toString(), decl2, decl2.getName(),
decl2.getType(), decl2.getType().toString()
The rule generates an alert on normal lambda functions.
Message: " Name _FUN uses the reserved prefix "
#include <iostream>
#include <string>
int main() {
auto is_digit = [](std::string& word) {
std::string::const_iterator it = word.begin();
while (it != word.end() && std::isdigit(*it)) ++it;
return !word.empty() && it == word.end();
};
std::string my_word("10");
auto test_word = is_digit(my_word);
return 0;
}
A5-2-2
Flagging c-style casts generated by macros can be confusing for the developer, because (a) it may not be clear where the cast is coming from and (b) the developer may not be able to address the finding, if the macro is defined in a library.
Some possible adjustments:
#define ADD_ONE(X) ((int)X) + 1
void example_function() {
int i = ADD_ONE(1);
}
Currently the implementation simply checks that the filename used in a call to fopen
is not tainted.
However, the rule requires that certain OS-supported dynamic checks are present in the code.
We'd like to enhance this query to support some usage pattern that would allow it to be excluded from being checked.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.