diff --git a/rules/S3518/cfamily/metadata.json b/rules/S3518/cfamily/metadata.json index 10c014e6e4..ff48e09418 100644 --- a/rules/S3518/cfamily/metadata.json +++ b/rules/S3518/cfamily/metadata.json @@ -11,6 +11,7 @@ "securityStandards": { "CERT": [ "NUM02-J.", + "INT32-C.", "INT33-C." ], "CWE": [ diff --git a/rules/S3518/cfamily/rule.adoc b/rules/S3518/cfamily/rule.adoc index b523ce02eb..c193c3905a 100644 --- a/rules/S3518/cfamily/rule.adoc +++ b/rules/S3518/cfamily/rule.adoc @@ -1,16 +1,149 @@ +Ensure that integer division and remainder operations do not result in divide-by-zero errors. + == Why is this an issue? -If the denominator to a division or modulo operation is zero it would result in a fatal error. +If the denominator to a division or modulo operation is zero, the behavior of the application is undefined. -include::../noncompliant.adoc[] +Operator `/` is used for division and `%` for modulo operation. +Division and modulo operations are susceptible to divide-by-zero (and signed integer overflow) errors. + +[source,cpp] +---- +int foo(int a, int b) { + if (b == 0) { + a = 1; + } + return a / b; // Noncompliant: potential divide-by-zero error +} +---- + + +== What is the potential impact? + +Integer division or remainder operations that result in divide-by-zero errors lead to *undefined behavior*. + +For programs that exercise undefined behavior, the compiler is no longer bound by the language specification. +The application may crash or, even worse, the application may appear to execute correctly while losing data or producing incorrect results. + + +== How to fix it + +Employ adequate checks that prevent divide-by-zero errors when using integer division or remainder operations. + +Alternatively, replace integer division with floating-point division for which the divide-by-zero case is well defined and does not introduce undefined behavior. + + +=== Code examples + +==== Noncompliant code example + +[source,cpp,diff-id=1,diff-type=noncompliant] +---- +#include +#include + +std::optional safe_division(int a, int b) { + // While the following check correctly prevents signed integer overflows, + // it fails to prevent divide-by-zero errors. If `b` is equal to `0`, the + // application emits undefined behavior. + if ((a == std::numeric_limits::min()) && (b == -1)) { + return std::nullopt; + } + return a / b; // Noncompliant: causes undefined behavior if `b` is zero +} + +std::optional foo(int a, int b) { + if (b == 0) { + a = 1; + } + return safe_division(a, b); +} +---- + +==== Compliant solution + +[source,cpp,diff-id=1,diff-type=compliant] +---- +#include +#include + +std::optional safe_division(int a, int b) { + if ((b == 0) || ((a == std::numeric_limits::min()) && (b == -1))) { + return std::nullopt; + } + return a / b; // Compliant: check correctly prevents divide-by-zero and signed integer overflows +} + +std::optional foo(int a, int b) { + if (b == 0) { + a = 1; + } + return safe_division(a, b); +} +---- + +[source,cpp,diff-id=1,diff-type=compliant] +---- +#include +#include + +std::optional safe_division(int a, int b) { + if ((a == std::numeric_limits::min()) && (b == -1)) { + return std::nullopt; + } + return a / b; // Compliant: check to prevent divide-by-zero in the caller +} + +std::optional foo(int a, int b) { + if (b == 0) { + return std::nullopt; + } + return safe_division(a, b); +} +---- + +[source,cpp,diff-id=1,diff-type=compliant] +---- +double foo(int a, int b) { + return a / static_cast(b); // Compliant: replace integer division by floating-point division +} +---- + +=== Going the extra mile + +Besides divide-by-zero errors, signed integer division is susceptible to overflows, too. +When the dividend is the minimum value for the signed integer type and the divisor is equal to `-1` an overflow is provoked due to two's complement representation. +This frequently causes hard-to-track bugs. + +The checks shown in the following code snippet correctly protect the division operation against divide-by-zero _and_ signed integer overflow errors: + +[source,cpp] +---- +#include +#include + +std::optional safe_division(int a, int b) { + if ((b == 0) || ((a == std::numeric_limits::min()) && (b == -1))) { + return std::nullopt; + } + return a / b; // Compliant: prevents divide-by-zero _and_ signed integer overflows +} +---- -include::../compliant.adoc[] == Resources -* https://cwe.mitre.org/data/definitions/369[MITRE, CWE-369] - Divide by zero -* https://wiki.sei.cmu.edu/confluence/x/CTZGBQ[CERT, NUM02-J.] - Ensure that division and remainder operations do not result in divide-by-zero errors -* https://wiki.sei.cmu.edu/confluence/x/ftYxBQ[CERT, INT33-C.] - Ensure that division and remainder operations do not result in divide-by-zero errors +=== Standards + +* CERT - https://wiki.sei.cmu.edu/confluence/x/CTZGBQ[NUM02-J. Ensure that division and remainder operations do not result in divide-by-zero errors] +* CERT - https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow[INT32-C. Ensure that operations on signed integers do not result in overflow] +* CERT - https://wiki.sei.cmu.edu/confluence/x/ftYxBQ[INT33-C. Ensure that division and remainder operations do not result in divide-by-zero errors] +* CWE - https://cwe.mitre.org/data/definitions/369[369 - Divide by zero] + +=== External coding guidelines + +* {cpp} Core Guidelines https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md[ES.105: Don't divide by integer zero] + ifdef::env-github,rspecator-view[] diff --git a/rules/S3518/metadata.json b/rules/S3518/metadata.json index 6d783987b9..559015907b 100644 --- a/rules/S3518/metadata.json +++ b/rules/S3518/metadata.json @@ -36,5 +36,5 @@ "defaultQualityProfiles": [ "Sonar way" ], - "quickfix": "unknown" + "quickfix": "infeasible" }