Skip to content

Commit

Permalink
Modify rule S3518: Expand and adjust for LaYC
Browse files Browse the repository at this point in the history
## Review

A dedicated reviewer checked the rule description successfully for:

- [ ] logical errors and incorrect information
- [ ] information gaps and missing content
- [ ] text style and tone
- [ ] PR summary and labels follow [the
guidelines](https://github.com/SonarSource/rspec/#to-modify-an-existing-rule)

---------

Co-authored-by: Arseniy Zaostrovnykh <[email protected]>
  • Loading branch information
pdschbrt and necto authored Oct 4, 2023
1 parent a8e483f commit 3d55ef2
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 7 deletions.
1 change: 1 addition & 0 deletions rules/S3518/cfamily/metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"securityStandards": {
"CERT": [
"NUM02-J.",
"INT32-C.",
"INT33-C."
],
"CWE": [
Expand Down
145 changes: 139 additions & 6 deletions rules/S3518/cfamily/rule.adoc
Original file line number Diff line number Diff line change
@@ -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.

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 <limits>
#include <optional>
std::optional<int> 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<int>::min()) && (b == -1)) {
return std::nullopt;
}
return a / b; // Noncompliant: causes undefined behavior if `b` is zero
}
std::optional<int> foo(int a, int b) {
if (b == 0) {
a = 1;
}
return safe_division(a, b);
}
----

include::../noncompliant.adoc[]
==== Compliant solution

[source,cpp,diff-id=1,diff-type=compliant]
----
#include <limits>
#include <optional>
std::optional<int> safe_division(int a, int b) {
if ((b == 0) || ((a == std::numeric_limits<int>::min()) && (b == -1))) {
return std::nullopt;
}
return a / b; // Compliant: check correctly prevents divide-by-zero and signed integer overflows
}
std::optional<int> foo(int a, int b) {
if (b == 0) {
a = 1;
}
return safe_division(a, b);
}
----

[source,cpp,diff-id=1,diff-type=compliant]
----
#include <limits>
#include <optional>
std::optional<int> safe_division(int a, int b) {
if ((a == std::numeric_limits<int>::min()) && (b == -1)) {
return std::nullopt;
}
return a / b; // Compliant: check to prevent divide-by-zero in the caller
}
std::optional<int> 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<double>(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 <limits>
#include <optional>
std::optional<int> safe_division(int a, int b) {
if ((b == 0) || ((a == std::numeric_limits<int>::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[]

Expand Down
2 changes: 1 addition & 1 deletion rules/S3518/metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,5 @@
"defaultQualityProfiles": [
"Sonar way"
],
"quickfix": "unknown"
"quickfix": "infeasible"
}

0 comments on commit 3d55ef2

Please sign in to comment.