From cf2334088d2a5cd143c6dd4d8ffb9cc572968877 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource Date: Tue, 15 Oct 2024 12:09:32 +0000 Subject: [PATCH 1/5] Create rule S7128 --- rules/S7128/cfamily/metadata.json | 25 ++++++++++++++++++ rules/S7128/cfamily/rule.adoc | 44 +++++++++++++++++++++++++++++++ rules/S7128/metadata.json | 2 ++ 3 files changed, 71 insertions(+) create mode 100644 rules/S7128/cfamily/metadata.json create mode 100644 rules/S7128/cfamily/rule.adoc create mode 100644 rules/S7128/metadata.json diff --git a/rules/S7128/cfamily/metadata.json b/rules/S7128/cfamily/metadata.json new file mode 100644 index 00000000000..820072b4027 --- /dev/null +++ b/rules/S7128/cfamily/metadata.json @@ -0,0 +1,25 @@ +{ + "title": "FIXME", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-7128", + "sqKey": "S7128", + "scope": "All", + "defaultQualityProfiles": ["Sonar way"], + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "HIGH", + "RELIABILITY": "MEDIUM", + "SECURITY": "LOW" + }, + "attribute": "CONVENTIONAL" + } +} diff --git a/rules/S7128/cfamily/rule.adoc b/rules/S7128/cfamily/rule.adoc new file mode 100644 index 00000000000..69c2a03a830 --- /dev/null +++ b/rules/S7128/cfamily/rule.adoc @@ -0,0 +1,44 @@ +FIXME: add a description + +// If you want to factorize the description uncomment the following line and create the file. +//include::../description.adoc[] + +== Why is this an issue? + +FIXME: remove the unused optional headers (that are commented out) + +//=== What is the potential impact? + +== How to fix it +//== How to fix it in FRAMEWORK NAME + +=== Code examples + +==== Noncompliant code example + +[source,cpp,diff-id=1,diff-type=noncompliant] +---- +FIXME +---- + +==== Compliant solution + +[source,cpp,diff-id=1,diff-type=compliant] +---- +FIXME +---- + +//=== How does this work? + +//=== Pitfalls + +//=== Going the extra mile + + +//== Resources +//=== Documentation +//=== Articles & blog posts +//=== Conference presentations +//=== Standards +//=== External coding guidelines +//=== Benchmarks diff --git a/rules/S7128/metadata.json b/rules/S7128/metadata.json new file mode 100644 index 00000000000..2c63c085104 --- /dev/null +++ b/rules/S7128/metadata.json @@ -0,0 +1,2 @@ +{ +} From 83a4d4b87c215f6fff5a74356aa03d42835d7686 Mon Sep 17 00:00:00 2001 From: Tomasz Kaminski Date: Tue, 15 Oct 2024 15:21:40 +0200 Subject: [PATCH 2/5] Added rule descrption --- rules/S7128/cfamily/metadata.json | 9 ++-- rules/S7128/cfamily/rule.adoc | 71 +++++++++++++++++++++---------- 2 files changed, 53 insertions(+), 27 deletions(-) diff --git a/rules/S7128/cfamily/metadata.json b/rules/S7128/cfamily/metadata.json index 820072b4027..21c4c69edde 100644 --- a/rules/S7128/cfamily/metadata.json +++ b/rules/S7128/cfamily/metadata.json @@ -1,5 +1,5 @@ { - "title": "FIXME", + "title": "\"strlen\" should not be called on incremented pointer", "type": "CODE_SMELL", "status": "ready", "remediation": { @@ -8,18 +8,17 @@ }, "tags": [ ], - "defaultSeverity": "Major", + "defaultSeverity": "Minor", "ruleSpecification": "RSPEC-7128", "sqKey": "S7128", "scope": "All", "defaultQualityProfiles": ["Sonar way"], - "quickfix": "unknown", + "quickfix": "infeasible", "code": { "impacts": { - "MAINTAINABILITY": "HIGH", "RELIABILITY": "MEDIUM", "SECURITY": "LOW" }, - "attribute": "CONVENTIONAL" + "attribute": "CLEAR" } } diff --git a/rules/S7128/cfamily/rule.adoc b/rules/S7128/cfamily/rule.adoc index 69c2a03a830..29b731151c6 100644 --- a/rules/S7128/cfamily/rule.adoc +++ b/rules/S7128/cfamily/rule.adoc @@ -1,44 +1,71 @@ -FIXME: add a description - -// If you want to factorize the description uncomment the following line and create the file. -//include::../description.adoc[] +This rule raise issue of calls `strlen(ptr + 1)`, for which the intent is unclear. == Why is this an issue? -FIXME: remove the unused optional headers (that are commented out) +The call `strlen(ptr) + 1` is commonly used to compute memory required to copy a string: +the one is added to preserve space for having null-terminator. -//=== What is the potential impact? +The very similiar call `strlen(ptr + 1)` that differs by placement of the right parenthesis, +produces a vastly different results: + * non-empty string computes the lenght of string without first character (reduced by one) + * if string is empty, traverse memory after it until null terminator (`'\0'`) is found. +The later may lead to undefined behavior. -== How to fix it -//== How to fix it in FRAMEWORK NAME +Due above, the `strlen(ptr + 1)` calls are likely to be unintended, +and result from the type. This rule raises issue on such calls. === Code examples ==== Noncompliant code example -[source,cpp,diff-id=1,diff-type=noncompliant] +[source,c] ---- -FIXME +size_t size = strlen(ptr + 1); // Noncompliant ---- ==== Compliant solution -[source,cpp,diff-id=1,diff-type=compliant] +If the use of `strlen(ptr + 1)` was an typo: + +[source,c] +---- +size_t size = strlen(ptr) + 1; // Compliant +---- + +In case when the `strlen(ptr + 1)` was intentional, +to preserve same behavior you may incerement the pointer before the call. + +[source,c] +---- +char const* next = ptr + 1; +size_t size = strlen(next); // Compliant +---- + +This will convey the intent more cleanly, and avoid any suspiction regarding the typo. + +=== What is the potential impact? + +If the `strlen(ptr + 1)` is accidentally used instead of `strlen(ptr) + 1`, may have vast range of effects. +For example, it can lead to buffer overlflow it the operation was used to compute required memory: + +[source,c] ---- -FIXME +char* duplicate(char const* source) { + char result = (char*)malloc(strlen(source + 1)); // Should be malloc(strlen(source) + 1)) + strcpy(result, source); // Writes two characters outside of `result` buffer + return result; +} ---- -//=== How does this work? +Buffer overflow, as other undefined behavior, has a wide range of effects. +In many cases, the access works by accident and succeeds at writing or reading a value. +However, it can start misbehaving at any time. +If compilation flags, compiler, platform, or runtime environment change, +the same code can crash the application, corrupt memory, or leak a secret. -//=== Pitfalls +=== Going extra mile. -//=== Going the extra mile +{cpp} offers `std::string`, `std::string_view` and many other functions simplifying operating on string. +whne possible, higher-level level features shall be preffered over C-strings APIs. -//== Resources -//=== Documentation -//=== Articles & blog posts -//=== Conference presentations -//=== Standards -//=== External coding guidelines -//=== Benchmarks From b003d31c0e4ae208ecc9292ea48558fa9d1e0671 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Tue, 15 Oct 2024 15:37:33 +0200 Subject: [PATCH 3/5] Grammarly pass --- rules/S7128/cfamily/rule.adoc | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/rules/S7128/cfamily/rule.adoc b/rules/S7128/cfamily/rule.adoc index 29b731151c6..2e5b854c6fa 100644 --- a/rules/S7128/cfamily/rule.adoc +++ b/rules/S7128/cfamily/rule.adoc @@ -1,18 +1,18 @@ -This rule raise issue of calls `strlen(ptr + 1)`, for which the intent is unclear. +This rule raises the issue of calls `strlen(ptr + 1)`, for which the intent is unclear. == Why is this an issue? -The call `strlen(ptr) + 1` is commonly used to compute memory required to copy a string: -the one is added to preserve space for having null-terminator. +The call `strlen(ptr) + 1` is commonly used to compute the memory required to copy a string: +the one is added to preserve space for having a null-terminator. -The very similiar call `strlen(ptr + 1)` that differs by placement of the right parenthesis, -produces a vastly different results: - * non-empty string computes the lenght of string without first character (reduced by one) - * if string is empty, traverse memory after it until null terminator (`'\0'`) is found. -The later may lead to undefined behavior. +The very similar call `strlen(ptr + 1)` that differs by placement of the right parenthesis, +produces vastly different results: + * non-empty string computes the length of string without the first character (reduced by one) + * if the string is empty, traverse memory after it until the null terminator (`'\0'`) is found. +The latter may lead to undefined behavior. -Due above, the `strlen(ptr + 1)` calls are likely to be unintended, -and result from the type. This rule raises issue on such calls. +Due above, the `strlen(ptr + 1)` calls are likely to be unintended +and result from the type. This rule raises issues on such calls. === Code examples @@ -32,8 +32,7 @@ If the use of `strlen(ptr + 1)` was an typo: size_t size = strlen(ptr) + 1; // Compliant ---- -In case when the `strlen(ptr + 1)` was intentional, -to preserve same behavior you may incerement the pointer before the call. +In case when the `strlen(ptr + 1)` was intentional, to preserve the same behavior, you may increment the pointer before the call. [source,c] ---- @@ -41,18 +40,18 @@ char const* next = ptr + 1; size_t size = strlen(next); // Compliant ---- -This will convey the intent more cleanly, and avoid any suspiction regarding the typo. +This will convey the intent more clearly and avoid any uncertainty regarding the intent. === What is the potential impact? -If the `strlen(ptr + 1)` is accidentally used instead of `strlen(ptr) + 1`, may have vast range of effects. -For example, it can lead to buffer overlflow it the operation was used to compute required memory: +If `strlen(ptr + 1)` is accidentally used instead of `strlen(ptr) + 1`, it may have a wide range of effects. +For example, it can lead to buffer overflow if the operation was used to compute the required memory: [source,c] ---- char* duplicate(char const* source) { char result = (char*)malloc(strlen(source + 1)); // Should be malloc(strlen(source) + 1)) - strcpy(result, source); // Writes two characters outside of `result` buffer + strcpy(result, source); // Writes two characters outside of "result" buffer return result; } ---- @@ -65,7 +64,7 @@ the same code can crash the application, corrupt memory, or leak a secret. === Going extra mile. -{cpp} offers `std::string`, `std::string_view` and many other functions simplifying operating on string. -whne possible, higher-level level features shall be preffered over C-strings APIs. +{cpp} offers `std::string`, `std::string_view` and many other functions simplifying operating on strings. +When possible, higher-level level features shall be preferred over C-strings APIs. From 4a041b721bdd82eb1767eb498f5d61677e38148b Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:03:17 +0200 Subject: [PATCH 4/5] Update rule.adoc --- rules/S7128/cfamily/rule.adoc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rules/S7128/cfamily/rule.adoc b/rules/S7128/cfamily/rule.adoc index 2e5b854c6fa..677047c66fd 100644 --- a/rules/S7128/cfamily/rule.adoc +++ b/rules/S7128/cfamily/rule.adoc @@ -3,12 +3,14 @@ This rule raises the issue of calls `strlen(ptr + 1)`, for which the intent is u == Why is this an issue? The call `strlen(ptr) + 1` is commonly used to compute the memory required to copy a string: -the one is added to preserve space for having a null-terminator. +the one is needed to allocate space for a null-terminator. The very similar call `strlen(ptr + 1)` that differs by placement of the right parenthesis, produces vastly different results: + * non-empty string computes the length of string without the first character (reduced by one) * if the string is empty, traverse memory after it until the null terminator (`'\0'`) is found. + The latter may lead to undefined behavior. Due above, the `strlen(ptr + 1)` calls are likely to be unintended @@ -65,6 +67,6 @@ the same code can crash the application, corrupt memory, or leak a secret. === Going extra mile. {cpp} offers `std::string`, `std::string_view` and many other functions simplifying operating on strings. -When possible, higher-level level features shall be preferred over C-strings APIs. +When possible, higher-level features should be preferred over C-string APIs. From 76ce9bb3356d707d691e11a5b30cc0449b64c065 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:22:30 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Fred Tingaud <95592999+frederic-tingaud-sonarsource@users.noreply.github.com> --- rules/S7128/cfamily/metadata.json | 4 ++-- rules/S7128/cfamily/rule.adoc | 22 +++++++++++----------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/rules/S7128/cfamily/metadata.json b/rules/S7128/cfamily/metadata.json index 21c4c69edde..f87a46740cb 100644 --- a/rules/S7128/cfamily/metadata.json +++ b/rules/S7128/cfamily/metadata.json @@ -1,5 +1,5 @@ { - "title": "\"strlen\" should not be called on incremented pointer", + "title": "\"strlen\" should not be called on incremented pointers", "type": "CODE_SMELL", "status": "ready", "remediation": { @@ -8,7 +8,7 @@ }, "tags": [ ], - "defaultSeverity": "Minor", + "defaultSeverity": "Major", "ruleSpecification": "RSPEC-7128", "sqKey": "S7128", "scope": "All", diff --git a/rules/S7128/cfamily/rule.adoc b/rules/S7128/cfamily/rule.adoc index 677047c66fd..9544a6e091b 100644 --- a/rules/S7128/cfamily/rule.adoc +++ b/rules/S7128/cfamily/rule.adoc @@ -1,26 +1,26 @@ -This rule raises the issue of calls `strlen(ptr + 1)`, for which the intent is unclear. +Calling `strlen(ptr + 1)` looks very similar to the more common pattern `strlen(ptr) + 1` and has unclear intent. == Why is this an issue? The call `strlen(ptr) + 1` is commonly used to compute the memory required to copy a string: -the one is needed to allocate space for a null-terminator. +the `+ 1` is needed to allocate space for a null-terminator. -The very similar call `strlen(ptr + 1)` that differs by placement of the right parenthesis, +The very similar call `strlen(ptr + 1)` that differs only by the placement of the right parenthesis, produces vastly different results: - * non-empty string computes the length of string without the first character (reduced by one) - * if the string is empty, traverse memory after it until the null terminator (`'\0'`) is found. + * non-empty string computes the length of string without the first character (`strlen(ptr) - 1`) + * if the string is empty, it traverses neighboring memory until a null terminator (`'\0'`) is found. The latter may lead to undefined behavior. -Due above, the `strlen(ptr + 1)` calls are likely to be unintended -and result from the type. This rule raises issues on such calls. +In consequence, calls to `strlen(ptr + 1)` are likely to be unintended +and the result of a typo. This rule raises issues on such calls. === Code examples ==== Noncompliant code example -[source,c] +[source,c,diff-id=1,diff-type=noncompliant] ---- size_t size = strlen(ptr + 1); // Noncompliant ---- @@ -29,7 +29,7 @@ size_t size = strlen(ptr + 1); // Noncompliant If the use of `strlen(ptr + 1)` was an typo: -[source,c] +[source,c,diff-id=1,diff-type=compliant] ---- size_t size = strlen(ptr) + 1; // Compliant ---- @@ -52,13 +52,13 @@ For example, it can lead to buffer overflow if the operation was used to compute [source,c] ---- char* duplicate(char const* source) { - char result = (char*)malloc(strlen(source + 1)); // Should be malloc(strlen(source) + 1)) + char* result = (char*)malloc(strlen(source + 1)); // Should be malloc(strlen(source) + 1)) strcpy(result, source); // Writes two characters outside of "result" buffer return result; } ---- -Buffer overflow, as other undefined behavior, has a wide range of effects. +Buffer overflows, as other undefined behavior, have a wide range of effects. In many cases, the access works by accident and succeeds at writing or reading a value. However, it can start misbehaving at any time. If compilation flags, compiler, platform, or runtime environment change,