From 8e830bc3f5cae97e7169b5d7f7a9c229abd35e26 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 23:00:53 +0100 Subject: [PATCH 1/2] RULE-1-3: Improve detection of standard compliant main functions - Improve alert message to report the undefined behavior triggered. - Address both false positives and false negatives in identifying standard compliant main methods. Previously, `void main()` was considered permitted and `int main(void)` banned. In addition, we now detect main methods as standard compliant if they use typedefs, and if arrays are used in the definition of `argv`. --- .../codingstandards/c/UndefinedBehavior.qll | 39 +++++++++++++++---- .../RULE-1-3/OccurrenceOfUndefinedBehavior.ql | 2 +- .../OccurrenceOfUndefinedBehavior.expected | 13 ++++--- c/misra/test/rules/RULE-1-3/test.c | 39 +++++++++++++++---- change_notes/2024-10-21-rule-1-3-main.md | 3 ++ 5 files changed, 76 insertions(+), 20 deletions(-) create mode 100644 change_notes/2024-10-21-rule-1-3-main.md diff --git a/c/common/src/codingstandards/c/UndefinedBehavior.qll b/c/common/src/codingstandards/c/UndefinedBehavior.qll index 5c9dc230d8..bbbb08678b 100644 --- a/c/common/src/codingstandards/c/UndefinedBehavior.qll +++ b/c/common/src/codingstandards/c/UndefinedBehavior.qll @@ -6,28 +6,53 @@ import codingstandards.cpp.UndefinedBehavior */ abstract class CUndefinedBehavior extends UndefinedBehavior { } +class PointerOrArrayType extends DerivedType { + PointerOrArrayType() { + this instanceof PointerType or + this instanceof ArrayType + } +} + +Type get(Function main) { + main.getName() = "main" and + main.getNumberOfParameters() = 2 and + main.getType().getUnderlyingType() instanceof IntType and + main.getParameter(0).getType().getUnderlyingType() instanceof IntType and + result = main.getParameter(1).getType().getUnderlyingType().(PointerOrArrayType).getBaseType() +} + +/** + * A function which has the signature - but not the name - of a main function. + */ class C99MainFunction extends Function { C99MainFunction() { this.getNumberOfParameters() = 2 and - this.getType() instanceof IntType and - this.getParameter(0).getType() instanceof IntType and - this.getParameter(1).getType().(PointerType).getBaseType().(PointerType).getBaseType() - instanceof CharType + this.getType().getUnderlyingType() instanceof IntType and + this.getParameter(0).getType().getUnderlyingType() instanceof IntType and + this.getParameter(1) + .getType() + .getUnderlyingType() + .(PointerOrArrayType) + .getBaseType() + .(PointerOrArrayType) + .getBaseType() instanceof CharType or this.getNumberOfParameters() = 0 and - this.getType() instanceof VoidType + // Must be explicitly declared as `int main(void)`. + this.getADeclarationEntry().hasVoidParamList() and + this.getType().getUnderlyingType() instanceof IntType } } class CUndefinedMainDefinition extends CUndefinedBehavior, Function { CUndefinedMainDefinition() { // for testing purposes, we use the prefix ____codeql_coding_standards` - (this.getName() = "main" or this.getName().indexOf("____codeql_coding_standards") = 0) and + (this.getName() = "main" or this.getName().indexOf("____codeql_coding_standards_main") = 0) and not this instanceof C99MainFunction } override string getReason() { result = - "The behavior of the program is undefined because the main function is not defined according to the C standard." + "main function may trigger undefined behavior because it is not in one of the formats specified by the C standard." } } diff --git a/c/misra/src/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.ql b/c/misra/src/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.ql index 53f72e6bee..00ef875985 100644 --- a/c/misra/src/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.ql +++ b/c/misra/src/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.ql @@ -18,4 +18,4 @@ import codingstandards.c.UndefinedBehavior from CUndefinedBehavior c where not isExcluded(c, Language3Package::occurrenceOfUndefinedBehaviorQuery()) -select c, "May result in undefined behavior." +select c, c.getReason() diff --git a/c/misra/test/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.expected b/c/misra/test/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.expected index 68216d500f..1e57f92e4a 100644 --- a/c/misra/test/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.expected +++ b/c/misra/test/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.expected @@ -1,5 +1,8 @@ -| test.c:8:6:8:35 | ____codeql_coding_standards_m2 | May result in undefined behavior. | -| test.c:11:5:11:34 | ____codeql_coding_standards_m3 | May result in undefined behavior. | -| test.c:15:5:15:34 | ____codeql_coding_standards_m4 | May result in undefined behavior. | -| test.c:19:5:19:34 | ____codeql_coding_standards_m5 | May result in undefined behavior. | -| test.c:23:5:23:34 | ____codeql_coding_standards_m6 | May result in undefined behavior. | +| test.c:4:6:4:38 | ____codeql_coding_standards_main1 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | +| test.c:8:5:8:37 | ____codeql_coding_standards_main2 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | +| test.c:27:5:27:37 | ____codeql_coding_standards_main6 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | +| test.c:32:6:32:38 | ____codeql_coding_standards_main7 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | +| test.c:36:5:36:37 | ____codeql_coding_standards_main8 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | +| test.c:40:5:40:37 | ____codeql_coding_standards_main9 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | +| test.c:44:5:44:38 | ____codeql_coding_standards_main10 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | +| test.c:48:5:48:38 | ____codeql_coding_standards_main11 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. | diff --git a/c/misra/test/rules/RULE-1-3/test.c b/c/misra/test/rules/RULE-1-3/test.c index 190cff4000..fd54959f56 100644 --- a/c/misra/test/rules/RULE-1-3/test.c +++ b/c/misra/test/rules/RULE-1-3/test.c @@ -1,25 +1,50 @@ -void main(void) { // COMPLIANT +int main(void) { // COMPLIANT } -int ____codeql_coding_standards_m1(int argc, char **argv) { // NON_COMPLIANT +void ____codeql_coding_standards_main1(void) { // NON_COMPLIANT return 0; } -void ____codeql_coding_standards_m2(char *argc, char **argv) { // NON_COMPLIANT +int ____codeql_coding_standards_main2() { // NON_COMPLIANT + return 0; +} + +int ____codeql_coding_standards_main3(int argc, char **argv) { // COMPLIANT + return 0; +} + +int ____codeql_coding_standards_main4(int argc, char argv[][]) { // COMPLIANT + return 0; +} + +int ____codeql_coding_standards_main5(int argc, char *argv[]) { // COMPLIANT + return 0; +} + +typedef int MY_INT; +typedef char *MY_CHAR_PTR; + +int ____codeql_coding_standards_main6(MY_INT argc, + MY_CHAR_PTR argv[]) { // COMPLIANT + return 0; +} + +void ____codeql_coding_standards_main7(char *argc, + char **argv) { // NON_COMPLIANT } -int ____codeql_coding_standards_m3(int argc, char *argv) { // NON_COMPLIANT +int ____codeql_coding_standards_main8(int argc, char *argv) { // NON_COMPLIANT return 0; } -int ____codeql_coding_standards_m4() { // NON_COMPLIANT +int ____codeql_coding_standards_main9() { // NON_COMPLIANT return 0; } -int ____codeql_coding_standards_m5(int argc, int *argv) { // NON_COMPLIANT +int ____codeql_coding_standards_main10(int argc, int *argv) { // NON_COMPLIANT return 0; } -int ____codeql_coding_standards_m6(int argc, int **argv) { // NON_COMPLIANT +int ____codeql_coding_standards_main11(int argc, int **argv) { // NON_COMPLIANT return 0; } diff --git a/change_notes/2024-10-21-rule-1-3-main.md b/change_notes/2024-10-21-rule-1-3-main.md new file mode 100644 index 0000000000..7bd8d4bd54 --- /dev/null +++ b/change_notes/2024-10-21-rule-1-3-main.md @@ -0,0 +1,3 @@ + - `RULE-1-3` - `OccurrenceOfUndefinedBehavior.ql`: + - Improve alert message to report the undefined behavior triggered. + - Address both false positives and false negatives in identifying standard compliant main methods. Previously, `void main()` was considered permitted and `int main(void)` banned. In addition, we now detect main methods as standard compliant if they use typedefs, and if arrays are used in the definition of `argv`. \ No newline at end of file From 4b9d8136208c4cc002c24bba794d4c208663ebcf Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 22 Oct 2024 21:12:32 +0100 Subject: [PATCH 2/2] Share PointerOrArrayType --- .../codingstandards/c/UndefinedBehavior.qll | 20 +++---------------- .../src/codingstandards/cpp/Pointers.qll | 12 ++++++++++- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/c/common/src/codingstandards/c/UndefinedBehavior.qll b/c/common/src/codingstandards/c/UndefinedBehavior.qll index bbbb08678b..6a72cb6eb7 100644 --- a/c/common/src/codingstandards/c/UndefinedBehavior.qll +++ b/c/common/src/codingstandards/c/UndefinedBehavior.qll @@ -1,4 +1,5 @@ import cpp +import codingstandards.cpp.Pointers import codingstandards.cpp.UndefinedBehavior /** @@ -6,21 +7,6 @@ import codingstandards.cpp.UndefinedBehavior */ abstract class CUndefinedBehavior extends UndefinedBehavior { } -class PointerOrArrayType extends DerivedType { - PointerOrArrayType() { - this instanceof PointerType or - this instanceof ArrayType - } -} - -Type get(Function main) { - main.getName() = "main" and - main.getNumberOfParameters() = 2 and - main.getType().getUnderlyingType() instanceof IntType and - main.getParameter(0).getType().getUnderlyingType() instanceof IntType and - result = main.getParameter(1).getType().getUnderlyingType().(PointerOrArrayType).getBaseType() -} - /** * A function which has the signature - but not the name - of a main function. */ @@ -32,9 +18,9 @@ class C99MainFunction extends Function { this.getParameter(1) .getType() .getUnderlyingType() - .(PointerOrArrayType) + .(UnspecifiedPointerOrArrayType) .getBaseType() - .(PointerOrArrayType) + .(UnspecifiedPointerOrArrayType) .getBaseType() instanceof CharType or this.getNumberOfParameters() = 0 and diff --git a/cpp/common/src/codingstandards/cpp/Pointers.qll b/cpp/common/src/codingstandards/cpp/Pointers.qll index 8ed55b2bc0..28b6abc340 100644 --- a/cpp/common/src/codingstandards/cpp/Pointers.qll +++ b/cpp/common/src/codingstandards/cpp/Pointers.qll @@ -6,7 +6,7 @@ import cpp import codingstandards.cpp.Type /** - * A type that is a pointer or array type. + * A type that is a pointer or array type after stripping top-level specifiers. */ class PointerOrArrayType extends DerivedType { PointerOrArrayType() { @@ -15,6 +15,16 @@ class PointerOrArrayType extends DerivedType { } } +/** + * A type that is a pointer or array type. + */ +class UnspecifiedPointerOrArrayType extends DerivedType { + UnspecifiedPointerOrArrayType() { + this instanceof PointerType or + this instanceof ArrayType + } +} + /** * An expression which performs pointer arithmetic */