diff --git a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql index abd22068dd..36b946491b 100644 --- a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql +++ b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql @@ -15,8 +15,56 @@ import cpp import codingstandards.c.misra +import codingstandards.cpp.Macro +import codingstandards.cpp.Includes +import codingstandards.cpp.PreprocessorDirective -from Macro m, Macro m2 +/** + * Gets a top level element that this macro is expanded to, e.g. an element which does not also have + * an enclosing element in the macro. + */ +Element getATopLevelElement(MacroInvocation mi) { + result = mi.getAnExpandedElement() and + not result.getEnclosingElement() = mi.getAnExpandedElement() and + not result instanceof Conversion +} + +/** + * Gets a link target that this macro is expanded in. + */ +LinkTarget getALinkTarget(Macro m) { + exists(MacroInvocation mi, Element e | + mi = m.getAnInvocation() and + e = getATopLevelElement(mi) + | + result = e.(Expr).getEnclosingFunction().getALinkTarget() + or + result = e.(Stmt).getEnclosingFunction().getALinkTarget() + or + exists(GlobalOrNamespaceVariable g | + result = g.getALinkTarget() and + g = e.(Expr).getEnclosingDeclaration() + ) + ) +} + +/** + * Holds if the m1 and m2 are unconditionally included from a common file. + * + * Extracted out for performance reasons - otherwise the call to determine the file path for the + * message was specializing the calls to `getAnUnconditionallyIncludedFile*(..)` and causing + * slow performance. + */ +bindingset[m1, m2] +pragma[inline_late] +private predicate isIncludedUnconditionallyFromCommonFile(Macro m1, Macro m2) { + exists(File f | + getAnUnconditionallyIncludedFile*(f) = m1.getFile() and + getAnUnconditionallyIncludedFile*(f) = m2.getFile() + ) +} + +from Macro m, Macro m2, string message where not isExcluded(m, Declarations1Package::macroIdentifiersNotDistinctQuery()) and not m = m2 and @@ -25,12 +73,40 @@ where //C90 states the first 31 characters of macro identifiers are significant and is not currently considered by this rule //ie an identifier differing on the 32nd character would be indistinct for C90 but distinct for C99 //and is currently not reported by this rule - if m.getName().length() >= 64 - then m.getName().prefix(63) = m2.getName().prefix(63) - else m.getName() = m2.getName() + if m.getName().length() >= 64 and not m.getName() = m2.getName() + then ( + m.getName().prefix(63) = m2.getName().prefix(63) and + message = + "Macro identifer " + m.getName() + " is nondistinct in first 63 characters, compared to $@." + ) else ( + m.getName() = m2.getName() and + message = + "Definition of macro " + m.getName() + + " is not distinct from alternative definition of $@ in " + + m2.getLocation().getFile().getRelativePath() + "." + ) ) and //reduce double report since both macros are in alert, arbitrary ordering - m.getLocation().getStartLine() >= m2.getLocation().getStartLine() -select m, - "Macro identifer " + m.getName() + " is nondistinct in first 63 characters, compared to $@.", m2, - m2.getName() + m.getLocation().getStartLine() >= m2.getLocation().getStartLine() and + // Not within an #ifndef MACRO_NAME + not exists(PreprocessorIfndef ifBranch | + m.getAGuard() = ifBranch or + m2.getAGuard() = ifBranch + | + ifBranch.getHead() = m.getName() + ) and + // Must be included unconditionally from the same file, otherwise m1 may not be defined + // when m2 is defined + isIncludedUnconditionallyFromCommonFile(m, m2) and + // Macros can't be mutually exclusive + not mutuallyExclusiveBranchDirectiveMacros(m, m2) and + not mutuallyExclusiveBranchDirectiveMacros(m2, m) and + // If at least one invocation exists for at least one of the macros, then they must share a link + // target - i.e. must both be expanded in the same context + ( + (exists(m.getAnInvocation()) and exists(m2.getAnInvocation())) + implies + // Must share a link target - e.g. must both be expanded in the same context + getALinkTarget(m) = getALinkTarget(m2) + ) +select m, message, m2, m2.getName() diff --git a/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected b/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected index 12507b2d3f..d44164d116 100644 --- a/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected +++ b/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected @@ -1,2 +1,4 @@ +| header3.h:7:1:7:24 | #define MULTIPLE_INCLUDE | Definition of macro MULTIPLE_INCLUDE is not distinct from alternative definition of $@ in rules/RULE-5-4/header4.h. | header4.h:1:1:1:24 | #define MULTIPLE_INCLUDE | MULTIPLE_INCLUDE | +| header3.h:14:1:14:21 | #define NOT_PROTECTED | Definition of macro NOT_PROTECTED is not distinct from alternative definition of $@ in rules/RULE-5-4/header4.h. | header4.h:12:1:12:23 | #define NOT_PROTECTED 1 | NOT_PROTECTED | | test.c:2:1:2:72 | #define iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyB | Macro identifer iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyB is nondistinct in first 63 characters, compared to $@. | test.c:1:1:1:72 | #define iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyA | iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyA | -| test.c:8:1:8:31 | #define FUNCTION_MACRO(X) X + 1 | Macro identifer FUNCTION_MACRO is nondistinct in first 63 characters, compared to $@. | test.c:7:1:7:57 | #define FUNCTION_MACRO(FUNCTION_MACRO) FUNCTION_MACRO + 1 | FUNCTION_MACRO | +| test.c:8:1:8:31 | #define FUNCTION_MACRO(X) X + 1 | Definition of macro FUNCTION_MACRO is not distinct from alternative definition of $@ in rules/RULE-5-4/test.c. | test.c:7:1:7:57 | #define FUNCTION_MACRO(FUNCTION_MACRO) FUNCTION_MACRO + 1 | FUNCTION_MACRO | diff --git a/c/misra/test/rules/RULE-5-4/conditional.h b/c/misra/test/rules/RULE-5-4/conditional.h new file mode 100644 index 0000000000..d30701c8e0 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/conditional.h @@ -0,0 +1,11 @@ +#ifdef FOO +#include "header1.h" +#else +#include "header2.h" +#endif + +#ifdef FOO +#define A_MACRO 1 // COMPLIANT +#else +#define A_MACRO 2 // COMPLIANT +#endif \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header1.h b/c/misra/test/rules/RULE-5-4/header1.h new file mode 100644 index 0000000000..526f4fa659 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header1.h @@ -0,0 +1 @@ +#define REPEATED 11 // COMPLIANT \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header2.h b/c/misra/test/rules/RULE-5-4/header2.h new file mode 100644 index 0000000000..bd5dde123d --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header2.h @@ -0,0 +1 @@ +#define REPEATED 1 // COMPLIANT \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header3.h b/c/misra/test/rules/RULE-5-4/header3.h new file mode 100644 index 0000000000..3aa82f5fd7 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header3.h @@ -0,0 +1,16 @@ +#ifndef HEADER3_H +#define HEADER3_H + +// We should ignore the header guards in this file + +// This is defined unconditionally by both header3.h and header4.h +#define MULTIPLE_INCLUDE // NON_COMPLIANT + +// This is redefined in header3.h, but only if it isn't already defined +#define PROTECTED // COMPLIANT + +// This is redefined in header3.h, but is conditional on some other condition, +// so this is redefined +#define NOT_PROTECTED // NON_COMPLIANT + +#endif \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header4.h b/c/misra/test/rules/RULE-5-4/header4.h new file mode 100644 index 0000000000..8fa6e8b5e8 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header4.h @@ -0,0 +1,13 @@ +#define MULTIPLE_INCLUDE // NON_COMPLIANT + +// This case is triggered from root2.c +// because PROTECTED isn't defined in +// that case +#ifndef PROTECTED +#define PROTECTED // COMPLIANT - checked by guard +#endif + +// Always enabled, so conflicts in root1.c case +#ifdef MULTIPLE_INCLUDE +#define NOT_PROTECTED 1 // NON_COMPLIANT +#endif diff --git a/c/misra/test/rules/RULE-5-4/root1.c b/c/misra/test/rules/RULE-5-4/root1.c new file mode 100644 index 0000000000..98a94abed5 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/root1.c @@ -0,0 +1,6 @@ +#define FOO 1 +#include "conditional.h" + +// Both headers define MULTIPLE_INCLUDE +#include "header3.h" +#include "header4.h" \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/root2.c b/c/misra/test/rules/RULE-5-4/root2.c new file mode 100644 index 0000000000..39926b9b1e --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/root2.c @@ -0,0 +1,3 @@ +#include "conditional.h" + +#include "header4.h" \ No newline at end of file diff --git a/change_notes/2024-10-21-rule-5-4-conditional.md b/change_notes/2024-10-21-rule-5-4-conditional.md new file mode 100644 index 0000000000..cfc22f3642 --- /dev/null +++ b/change_notes/2024-10-21-rule-5-4-conditional.md @@ -0,0 +1,3 @@ + - `RULE-5-4` - `MacroIdentifiersNotDistinct.ql`: + - Exclude false positives related to conditional compilation, where a macro may be defined twice, but not within the same compilation. + - Improve alert message in the case the 63 char limit is not relevant by using the form "Definition of macro `` is not distinct from alternative definition of `` in ``. \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Includes.qll b/cpp/common/src/codingstandards/cpp/Includes.qll new file mode 100644 index 0000000000..c0c66ae2f5 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/Includes.qll @@ -0,0 +1,37 @@ +/** A library which supports analysis of includes. */ + +import cpp +import codingstandards.cpp.PreprocessorDirective +import semmle.code.cpp.headers.MultipleInclusion + +pragma[noinline] +private predicate hasIncludeLocation(Include include, string filepath, int startline) { + include.getLocation().hasLocationInfo(filepath, startline, _, _, _) +} + +/** + * Holds if `include` is included conditionally based on the branch directive `b1`. + */ +pragma[noinline] +predicate isConditionallyIncluded(PreprocessorBranchDirective bd, Include include) { + not bd = any(CorrectIncludeGuard c).getIfndef() and + not bd.getHead().regexpMatch(".*_H(_.*)?") and + exists(string filepath, int startline, int endline, int includeline | + isBranchDirectiveRange(bd, filepath, startline, endline) and + hasIncludeLocation(include, filepath, includeline) and + startline < includeline and + endline > includeline + ) +} + +/** + * Gets a file which is directly included from `fromFile` unconditionally. + */ +File getAnUnconditionallyIncludedFile(File fromFile) { + // Find an include which isn't conditional + exists(Include i | + i.getFile() = fromFile and + not isConditionallyIncluded(_, i) and + result = i.getIncludedFile() + ) +} diff --git a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll index fe619e5317..ca943742d9 100644 --- a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll +++ b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll @@ -40,3 +40,68 @@ class PreprocessorIfOrElif extends PreprocessorBranch { this instanceof PreprocessorElif } } + +/** + * Holds if the preprocessor directive `m` is located at `filepath` and `startline`. + */ +pragma[noinline] +predicate hasPreprocessorLocation(PreprocessorDirective m, string filepath, int startline) { + m.getLocation().hasLocationInfo(filepath, startline, _, _, _) +} + +/** + * Holds if `first` and `second` are a pair of branch directives in the same file, such that they + * share the same root if condition. + */ +pragma[noinline] +private predicate isBranchDirectivePair( + PreprocessorBranchDirective first, PreprocessorBranchDirective second, string filepath, + int b1StartLocation, int b2StartLocation +) { + first.getIf() = second.getIf() and + not first = second and + hasPreprocessorLocation(first, filepath, b1StartLocation) and + hasPreprocessorLocation(second, filepath, b2StartLocation) and + b1StartLocation < b2StartLocation +} + +/** + * Holds if `bd` is a branch directive in the range `filepath`, `startline`, `endline`. + */ +pragma[noinline] +predicate isBranchDirectiveRange( + PreprocessorBranchDirective bd, string filepath, int startline, int endline +) { + hasPreprocessorLocation(bd, filepath, startline) and + exists(PreprocessorBranchDirective next | + next = bd.getNext() and + // Avoid referencing filepath here, otherwise the optimiser will try to join + // on it + hasPreprocessorLocation(next, _, endline) + ) +} + +/** + * Holds if the macro `m` is defined within the branch directive `bd`. + */ +pragma[noinline] +predicate isMacroDefinedWithinBranch(PreprocessorBranchDirective bd, Macro m) { + exists(string filepath, int startline, int endline, int macroline | + isBranchDirectiveRange(bd, filepath, startline, endline) and + hasPreprocessorLocation(m, filepath, macroline) and + startline < macroline and + endline > macroline + ) +} + +/** + * Holds if the pair of macros are "conditional" i.e. only one of the macros is followed in any + * particular compilation of the containing file. + */ +predicate mutuallyExclusiveBranchDirectiveMacros(Macro firstMacro, Macro secondMacro) { + exists(PreprocessorBranchDirective b1, PreprocessorBranchDirective b2 | + isBranchDirectivePair(b1, b2, _, _, _) and + isMacroDefinedWithinBranch(b1, firstMacro) and + isMacroDefinedWithinBranch(b2, secondMacro) + ) +}