From 7a6b8df1ddce5778cb6b677abb9b82ee11f9cd2b Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 23 Jul 2023 18:13:04 +0100 Subject: [PATCH 1/5] RULE-12-2: Support RemAssignExpr Add %= support to our SimpleRangeAnalysisCustomizations. --- .../RightHandOperandOfAShiftRange.expected | 3 ++ c/misra/test/rules/RULE-12-2/test.c | 12 +++++++ .../cpp/SimpleRangeAnalysisCustomizations.qll | 35 +++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected b/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected index a4deb83a14..9e2d3e9e6c 100644 --- a/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected +++ b/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected @@ -8,3 +8,6 @@ | test.c:25:10:25:10 | 8 | The right hand operand of the shift operator shall lie in the range 0 to 7. | | test.c:26:10:26:11 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 7. | | test.c:30:16:30:17 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 63. | +| test.c:34:8:34:8 | y | The right hand operand of the shift operator shall lie in the range 0 to 31. | +| test.c:40:8:40:8 | y | The right hand operand of the shift operator shall lie in the range 0 to 31. | +| test.c:42:8:42:8 | y | The right hand operand of the shift operator shall lie in the range 0 to 31. | diff --git a/c/misra/test/rules/RULE-12-2/test.c b/c/misra/test/rules/RULE-12-2/test.c index 449a47b7ae..db7b7b062d 100644 --- a/c/misra/test/rules/RULE-12-2/test.c +++ b/c/misra/test/rules/RULE-12-2/test.c @@ -29,3 +29,15 @@ void f1() { ULONG_MAX << 8; // COMPLIANT ULONG_MAX << 64; // NON_COMPLIANT } + +void unsignedRemAssign(unsigned int y, unsigned int x) { + x >> y; // NON_COMPLIANT + y %= 32; + x >> y; // COMPLIANT +} + +void signedRemAssign(signed int y, signed int x) { + x >> y; // NON_COMPLIANT + y %= 32; + x >> y; // NON_COMPLIANT - may be negative +} \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll b/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll index 469fe9a738..038d09413e 100644 --- a/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll +++ b/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll @@ -151,6 +151,41 @@ private class CastEnumToIntegerSimpleRange extends SimpleRangeAnalysisExpr, Cast override predicate dependsOnChild(Expr child) { child = getExpr() } } +/** + * A range analysis extension that supports `%=`. + */ +private class RemAssignSimpleRange extends SimpleRangeAnalysisExpr, AssignRemExpr { + override float getLowerBounds() { + exists(float maxDivisorNegated, float dividendLowerBounds | + // Find the max divisor, negated e.g. `%= 32` would be `-31` + maxDivisorNegated = (getFullyConvertedUpperBounds(getRValue()).abs() - 1) * -1 and + // Find the lower bounds of the dividend + dividendLowerBounds = getFullyConvertedLowerBounds(getLValue()) and + // The lower bound is caluclated in two steps: + // 1. Determine the maximum of the dividend lower bound and maxDivisorNegated. + // When the dividend is negative this will result in a negative result + // 2. Find the minimum with 0. If the divided is always >0 this will produce 0 + // otherwise it will produce the lowest negative number that can be held + // after the modulo. + result = 0.minimum(dividendLowerBounds.maximum(maxDivisorNegated)) + ) + } + + override float getUpperBounds() { + // TODO rem zero? + exists(float maxDivisor, float maxDividend | + // The maximum divisor value is the absolute value of the divisor minus 1 + maxDivisor = getFullyConvertedUpperBounds(getRValue()).abs() - 1 and + // value if > 0 otherwise 0 + maxDividend = getFullyConvertedUpperBounds(getLValue()).maximum(0) and + // In the case the numerator is definitely less than zero, the result could be negative + result = maxDividend.minimum(maxDivisor) + ) + } + + override predicate dependsOnChild(Expr expr) { expr = getAChild() } +} + /** * functions that read a character from the STDIN, * or return EOF if it fails to do so. From 7b7389648627ef61e17ea8dfd9add3601ff10d23 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 23 Jul 2023 18:41:05 +0100 Subject: [PATCH 2/5] EssentialTypes: Fix integer constant types Integer constants are only subject to the stlr or utlr if they are type int, signed int or unsigned int, otherwise they have the standard type. Fixes issues with RULE 12.2. --- .../codingstandards/c/misra/EssentialTypes.qll | 18 +++++++++++------- c/misra/test/c/misra/EssentialTypes.expected | 3 +++ c/misra/test/c/misra/test.c | 6 ++++++ 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/c/misra/src/codingstandards/c/misra/EssentialTypes.qll b/c/misra/src/codingstandards/c/misra/EssentialTypes.qll index a1b8d6fdb0..7df233d2ab 100644 --- a/c/misra/src/codingstandards/c/misra/EssentialTypes.qll +++ b/c/misra/src/codingstandards/c/misra/EssentialTypes.qll @@ -355,13 +355,17 @@ class EssentialLiteral extends EssentialExpr, Literal { else ( if this.(CharLiteral).getCharacter().length() = 1 then result instanceof PlainCharType - else ( - getStandardType().(IntegralType).isSigned() and - result = stlr(this) - or - not getStandardType().(IntegralType).isSigned() and - result = utlr(this) - ) + else + exists(Type underlyingStandardType | + underlyingStandardType = getStandardType().getUnderlyingType() + | + if underlyingStandardType instanceof IntType + then + if underlyingStandardType.(IntType).isSigned() + then result = stlr(this) + else result = utlr(this) + else result = underlyingStandardType + ) ) } } diff --git a/c/misra/test/c/misra/EssentialTypes.expected b/c/misra/test/c/misra/EssentialTypes.expected index 8bf299bd63..d9245311da 100644 --- a/c/misra/test/c/misra/EssentialTypes.expected +++ b/c/misra/test/c/misra/EssentialTypes.expected @@ -38,3 +38,6 @@ | test.c:26:3:26:3 | f | float | float | essentially Floating type | | test.c:27:3:27:5 | f32 | float32_t | float32_t | essentially Floating type | | test.c:28:3:28:6 | cf32 | float | float | essentially Floating type | +| test.c:32:3:32:3 | 1 | signed char | signed char | essentially Signed type | +| test.c:33:3:33:4 | 1 | unsigned char | unsigned char | essentially Unsigned type | +| test.c:34:3:34:5 | 1 | unsigned long | unsigned long | essentially Unsigned type | diff --git a/c/misra/test/c/misra/test.c b/c/misra/test/c/misra/test.c index 8788f7e93a..d7064c4c12 100644 --- a/c/misra/test/c/misra/test.c +++ b/c/misra/test/c/misra/test.c @@ -26,4 +26,10 @@ void testCategoriesForComplexTypes() { f; // Should be essentially Floating type f32; // Should be essentially Floating type cf32; // Should be essentially Floating type +} + +void testConstants() { + 1; // Essentially signed char + 1U; // Essentially unsigned char + 1UL; // Essentially unsigned long } \ No newline at end of file From 12494a41e9e9b752cf9216a95305eb6490be9b7b Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 23 Jul 2023 20:05:02 +0100 Subject: [PATCH 3/5] RULE-12-2: Improve reporting Update the message to include the right operand range in addition to the valid shift range, the essential type of the left operand, and, if relevant, the macro in which the shift is defined. --- .../RightHandOperandOfAShiftRange.ql | 47 +++++++++++++++++-- .../RightHandOperandOfAShiftRange.expected | 25 +++++----- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/c/misra/src/rules/RULE-12-2/RightHandOperandOfAShiftRange.ql b/c/misra/src/rules/RULE-12-2/RightHandOperandOfAShiftRange.ql index 891ca1e82a..bd77bdacd2 100644 --- a/c/misra/src/rules/RULE-12-2/RightHandOperandOfAShiftRange.ql +++ b/c/misra/src/rules/RULE-12-2/RightHandOperandOfAShiftRange.ql @@ -20,14 +20,51 @@ class ShiftExpr extends BinaryBitwiseOperation { ShiftExpr() { this instanceof LShiftExpr or this instanceof RShiftExpr } } -from ShiftExpr e, Expr right, int max_val +MacroInvocation getAMacroInvocation(ShiftExpr se) { result.getAnExpandedElement() = se } + +Macro getPrimaryMacro(ShiftExpr se) { + exists(MacroInvocation mi | + mi = getAMacroInvocation(se) and + not exists(MacroInvocation otherMi | + otherMi = getAMacroInvocation(se) and otherMi.getParentInvocation() = mi + ) and + result = mi.getMacro() + ) +} + +from + ShiftExpr e, Expr right, int max_val, float lowerBound, float upperBound, Type essentialType, + string extraMessage, Locatable optionalPlaceholderLocation, string optionalPlaceholderMessage where not isExcluded(right, Contracts7Package::rightHandOperandOfAShiftRangeQuery()) and right = e.getRightOperand().getFullyConverted() and - max_val = (8 * getEssentialType(e.getLeftOperand()).getSize()) - 1 and + essentialType = getEssentialType(e.getLeftOperand()) and + max_val = (8 * essentialType.getSize()) - 1 and + upperBound = upperBound(right) and + lowerBound = lowerBound(right) and + ( + lowerBound < 0 or + upperBound > max_val + ) and + // If this shift happens inside a macro, then report the macro as well + // for easier validation ( - lowerBound(right) < 0 or - upperBound(right) > max_val + if exists(getPrimaryMacro(e)) + then + extraMessage = " from expansion of macro $@" and + exists(Macro m | + m = getPrimaryMacro(e) and + optionalPlaceholderLocation = m and + optionalPlaceholderMessage = m.getName() + ) + else ( + extraMessage = "" and + optionalPlaceholderLocation = e and + optionalPlaceholderMessage = "" + ) ) select right, - "The right hand operand of the shift operator shall lie in the range 0 to " + max_val + "." + "The possible range of the right operand of the shift operator (" + lowerBound + ".." + upperBound + + ") is outside the the valid shift range (0.." + max_val + + ") for the essential type of the left operand (" + essentialType + ")" + extraMessage + ".", + optionalPlaceholderLocation, optionalPlaceholderMessage diff --git a/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected b/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected index 9e2d3e9e6c..5ac6f8bfd4 100644 --- a/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected +++ b/c/misra/test/rules/RULE-12-2/RightHandOperandOfAShiftRange.expected @@ -1,13 +1,12 @@ -| test.c:8:10:8:10 | 8 | The right hand operand of the shift operator shall lie in the range 0 to 7. | -| test.c:9:10:9:11 | - ... | The right hand operand of the shift operator shall lie in the range 0 to 7. | -| test.c:10:10:10:14 | ... + ... | The right hand operand of the shift operator shall lie in the range 0 to 7. | -| test.c:11:10:11:14 | ... + ... | The right hand operand of the shift operator shall lie in the range 0 to 7. | -| test.c:13:21:13:22 | 16 | The right hand operand of the shift operator shall lie in the range 0 to 15. | -| test.c:16:9:16:9 | 8 | The right hand operand of the shift operator shall lie in the range 0 to 7. | -| test.c:21:9:21:10 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 63. | -| test.c:25:10:25:10 | 8 | The right hand operand of the shift operator shall lie in the range 0 to 7. | -| test.c:26:10:26:11 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 7. | -| test.c:30:16:30:17 | 64 | The right hand operand of the shift operator shall lie in the range 0 to 63. | -| test.c:34:8:34:8 | y | The right hand operand of the shift operator shall lie in the range 0 to 31. | -| test.c:40:8:40:8 | y | The right hand operand of the shift operator shall lie in the range 0 to 31. | -| test.c:42:8:42:8 | y | The right hand operand of the shift operator shall lie in the range 0 to 31. | +| test.c:8:10:8:10 | 8 | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:8:3:8:10 | ... >> ... | | +| test.c:9:10:9:11 | - ... | The possible range of the right operand of the shift operator (-1..-1) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:9:3:9:11 | ... >> ... | | +| test.c:10:10:10:14 | ... + ... | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:10:3:10:14 | ... >> ... | | +| test.c:11:10:11:14 | ... + ... | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (uint8_t). | test.c:11:3:11:14 | ... << ... | | +| test.c:13:21:13:22 | 16 | The possible range of the right operand of the shift operator (16..16) is outside the the valid shift range (0..15) for the essential type of the left operand (uint16_t). | test.c:13:3:13:22 | ... << ... | | +| test.c:16:9:16:9 | 8 | The possible range of the right operand of the shift operator (8..8) is outside the the valid shift range (0..7) for the essential type of the left operand (unsigned char). | test.c:16:3:16:9 | ... << ... | | +| test.c:21:9:21:10 | 64 | The possible range of the right operand of the shift operator (64..64) is outside the the valid shift range (0..63) for the essential type of the left operand (unsigned long). | test.c:21:3:21:10 | ... << ... | | +| test.c:26:10:26:11 | 64 | The possible range of the right operand of the shift operator (64..64) is outside the the valid shift range (0..63) for the essential type of the left operand (unsigned long). | test.c:26:3:26:11 | ... << ... | | +| test.c:30:16:30:17 | 64 | The possible range of the right operand of the shift operator (64..64) is outside the the valid shift range (0..63) for the essential type of the left operand (unsigned long). | test.c:30:3:30:17 | ... << ... | | +| test.c:34:8:34:8 | y | The possible range of the right operand of the shift operator (0..4294967295) is outside the the valid shift range (0..31) for the essential type of the left operand (unsigned int). | test.c:34:3:34:8 | ... >> ... | | +| test.c:40:8:40:8 | y | The possible range of the right operand of the shift operator (-2147483648..2147483647) is outside the the valid shift range (0..31) for the essential type of the left operand (signed int). | test.c:40:3:40:8 | ... >> ... | | +| test.c:42:8:42:8 | y | The possible range of the right operand of the shift operator (-31..31) is outside the the valid shift range (0..31) for the essential type of the left operand (signed int). | test.c:42:3:42:8 | ... >> ... | | From 3c223ace4e6c91e5302f8e851b10dd24c4bb9509 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 11 Sep 2024 22:57:12 +0100 Subject: [PATCH 4/5] Fix comments - Fix typo - Do not need to handle rem by zero --- .../codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll b/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll index 038d09413e..5144f63dc2 100644 --- a/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll +++ b/cpp/common/src/codingstandards/cpp/SimpleRangeAnalysisCustomizations.qll @@ -161,7 +161,7 @@ private class RemAssignSimpleRange extends SimpleRangeAnalysisExpr, AssignRemExp maxDivisorNegated = (getFullyConvertedUpperBounds(getRValue()).abs() - 1) * -1 and // Find the lower bounds of the dividend dividendLowerBounds = getFullyConvertedLowerBounds(getLValue()) and - // The lower bound is caluclated in two steps: + // The lower bound is calculated in two steps: // 1. Determine the maximum of the dividend lower bound and maxDivisorNegated. // When the dividend is negative this will result in a negative result // 2. Find the minimum with 0. If the divided is always >0 this will produce 0 @@ -172,7 +172,6 @@ private class RemAssignSimpleRange extends SimpleRangeAnalysisExpr, AssignRemExp } override float getUpperBounds() { - // TODO rem zero? exists(float maxDivisor, float maxDividend | // The maximum divisor value is the absolute value of the divisor minus 1 maxDivisor = getFullyConvertedUpperBounds(getRValue()).abs() - 1 and From 19b54e2c667b85ddf5b4b3ce2298a96baef6910d Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 16 Sep 2024 11:29:38 +0100 Subject: [PATCH 5/5] Add change note --- change_notes/2024-09-11-rule-12-2-improvements.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 change_notes/2024-09-11-rule-12-2-improvements.md diff --git a/change_notes/2024-09-11-rule-12-2-improvements.md b/change_notes/2024-09-11-rule-12-2-improvements.md new file mode 100644 index 0000000000..0e713a5088 --- /dev/null +++ b/change_notes/2024-09-11-rule-12-2-improvements.md @@ -0,0 +1,6 @@ +- `RULE-12-2` - `RightHandOperandOfAShiftRange.ql`: + - Reduce false positives related to ranges determined by `%=`. + - Reduce false positives for integer constants with explicit size suffix were incorrectly identified as smaller types. + - Improve explanation of results, providing additional information on types and size ranges. + - Combine results stemming from the expansion of a macro, where the result is not dependent on the context. + \ No newline at end of file