From 5fd5d153e84665c9550afaabc9e8e1fb7f04af06 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 12 Dec 2023 10:25:59 -0800 Subject: [PATCH 01/10] C6059: Add heuristics This warning is similar to C6053, so I decided to reuse the heuristics section from there. --- docs/code-quality/c6059.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index 8a12022eb05..4fbf286b8cf 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -77,6 +77,10 @@ void f( ) } ``` +## Heuristics + +This warning is sometimes reported on certain idioms guaranteed to be safe in practice. Because of the frequency and potential consequences of this defect, the analysis tool is biased in favor of finding potential issues instead of its typical bias of reducing noise. + ## See also - [`strncpy_s`, `_strncpy_s_l`, `wcsncpy_s`, `_wcsncpy_s_l`, `_mbsncpy_s`, `_mbsncpy_s_l`](../c-runtime-library/reference/strncpy-s-strncpy-s-l-wcsncpy-s-wcsncpy-s-l-mbsncpy-s-mbsncpy-s-l.md) From 2568f89c36dac4b5d2d0e823b542cd642f4c3f8c Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 13 Dec 2023 17:12:46 -0800 Subject: [PATCH 02/10] Update heuristics to show examples of false negatives. --- docs/code-quality/c6059.md | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index 4fbf286b8cf..0d6f0f57f16 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -27,8 +27,8 @@ The following code generates warning C6059: void f( ) { char szTarget[MAX]; - char *szState ="Washington"; - char *szCity="Redmond, "; + const char *szState ="Washington"; + const char *szCity="Redmond, "; strncpy(szTarget, szCity, MAX); szTarget[MAX -1] = '\0'; @@ -46,8 +46,8 @@ To correct this warning, use the correct number of characters to concatenate as void f( ) { char szTarget[MAX]; - char *szState ="Washington"; - char *szCity="Redmond, "; + const char *szState ="Washington"; + const char *szCity="Redmond, "; strncpy(szTarget, szCity, MAX); szTarget[MAX -1] = '\0'; @@ -63,8 +63,8 @@ To correct this warning using the safe string manipulation functions `strncpy_s` void f( ) { - char *szState ="Washington"; - char *szCity="Redmond, "; + const char *szState ="Washington"; + const char *szCity="Redmond, "; size_t nTargetSize = strlen(szState) + strlen(szCity) + 1; char *szTarget= new char[nTargetSize]; @@ -79,7 +79,26 @@ void f( ) ## Heuristics -This warning is sometimes reported on certain idioms guaranteed to be safe in practice. Because of the frequency and potential consequences of this defect, the analysis tool is biased in favor of finding potential issues instead of its typical bias of reducing noise. +This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. The analysis is also able to detect cases where after simple computations the buffer size is passed as the length parameter. This warning is not given if some other value is passed as the length parameter, even if that value is incorrect. + +For example, the following code does not generate warning C6059, even though it is also an incorrect buffer length. + +```cpp +#include +#define MAX 25 + +void f( ) +{ + char szTarget[MAX]; + const char *szState ="Washington"; + const char *szCity="Redmond, "; + + strncpy(szTarget, szCity, MAX); + szTarget[MAX -1] = '\0'; + strncat(szTarget, szState, MAX); // wrong size + // code ... +} +``` ## See also From 0f39bc4259859feb7e5d4c853352db4797287910 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 13 Dec 2023 17:15:22 -0800 Subject: [PATCH 03/10] Address Acrolinx suggestions --- docs/code-quality/c6059.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index 0d6f0f57f16..24c6a6b80f7 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -79,9 +79,9 @@ void f( ) ## Heuristics -This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. The analysis is also able to detect cases where after simple computations the buffer size is passed as the length parameter. This warning is not given if some other value is passed as the length parameter, even if that value is incorrect. +This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. The analysis is also able to detect cases where after simple computations the buffer size is passed as the length parameter. This warning isn't given if some other value is passed as the length parameter, even if that value is incorrect. -For example, the following code does not generate warning C6059, even though it is also an incorrect buffer length. +For example, the following code doesn't generate warning C6059, even though it also uses an incorrect buffer length. ```cpp #include From 1551be20012d49ef4d862ea814b788f9867f24a3 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 14 Dec 2023 13:17:56 -0800 Subject: [PATCH 04/10] Fix false negative example --- docs/code-quality/c6059.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index 24c6a6b80f7..d171304777c 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -95,7 +95,7 @@ void f( ) strncpy(szTarget, szCity, MAX); szTarget[MAX -1] = '\0'; - strncat(szTarget, szState, MAX); // wrong size + strncat(szTarget, szState, MAX - 1); // wrong size, but no warning // code ... } ``` From cab6b02981c5124c79127fcfd7e3e61bd25cee82 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 14 Dec 2023 13:24:35 -0800 Subject: [PATCH 05/10] Reword to make contrast between true positive and false negative clearer --- docs/code-quality/c6059.md | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index d171304777c..50574245c47 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -81,7 +81,26 @@ void f( ) This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. The analysis is also able to detect cases where after simple computations the buffer size is passed as the length parameter. This warning isn't given if some other value is passed as the length parameter, even if that value is incorrect. -For example, the following code doesn't generate warning C6059, even though it also uses an incorrect buffer length. +For example, let's consider the first example again. The following code generates warning C6059: + +```cpp +#include +#define MAX 25 + +void f( ) +{ + char szTarget[MAX]; + const char *szState ="Washington"; + const char *szCity="Redmond, "; + + strncpy(szTarget, szCity, MAX); + szTarget[MAX -1] = '\0'; + strncat(szTarget, szState, MAX); // wrong size + // code ... +} +``` + +If we make a small change, changing the `MAX` argument to `strncat` to `MAX - 1`, the warning goes away, even though the length calculation is still incorrect. ```cpp #include From 25f764b469b855b6b5896bb3890383cee1c3a208 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 14 Dec 2023 14:07:10 -0800 Subject: [PATCH 06/10] More review feedback --- docs/code-quality/c6059.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index 50574245c47..cb8507a73c5 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -1,7 +1,7 @@ --- description: "Learn more about: Warning C6059" title: Warning C6059 -ms.date: 10/04/2022 +ms.date: 12/14/2023 f1_keywords: ["C6059", "BAD_CONCATENATION", "__WARNING_BAD_CONCATENATION"] helpviewer_keywords: ["C6059"] ms.assetid: 343a4cd1-048a-4edf-bb4b-187097bb6093 @@ -81,7 +81,7 @@ void f( ) This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. The analysis is also able to detect cases where after simple computations the buffer size is passed as the length parameter. This warning isn't given if some other value is passed as the length parameter, even if that value is incorrect. -For example, let's consider the first example again. The following code generates warning C6059: +Consider the following code, generates warning C6059: ```cpp #include @@ -100,7 +100,7 @@ void f( ) } ``` -If we make a small change, changing the `MAX` argument to `strncat` to `MAX - 1`, the warning goes away, even though the length calculation is still incorrect. +The warning goes away by changing the `MAX` argument to `strncat` to `MAX - 1`, even though the length calculation is still incorrect. ```cpp #include From 3f0ae7e19b9ace10bc63c4873ec6f5295c197cb1 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 14 Dec 2023 14:47:57 -0800 Subject: [PATCH 07/10] Remove unneeded detail --- docs/code-quality/c6059.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index cb8507a73c5..750ba7fbbc8 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -79,7 +79,7 @@ void f( ) ## Heuristics -This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. The analysis is also able to detect cases where after simple computations the buffer size is passed as the length parameter. This warning isn't given if some other value is passed as the length parameter, even if that value is incorrect. +This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. This warning isn't given if some other value is passed as the length parameter, even if that value is incorrect. Consider the following code, generates warning C6059: From f2e6d0a9365ebed7b9dca5203eb426198b919271 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 14 Dec 2023 14:49:12 -0800 Subject: [PATCH 08/10] Add some extra detail to the intro --- docs/code-quality/c6059.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index 750ba7fbbc8..32e7a46f4a9 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -14,6 +14,8 @@ ms.assetid: 343a4cd1-048a-4edf-bb4b-187097bb6093 This warning indicates that a call to a string concatenation function is probably passing an incorrect value for the number of characters to concatenate. This defect might cause an exploitable buffer overrun or crash. A common cause of this defect is passing the buffer size (instead of the remaining number of characters in the buffer) to the string manipulation function. +This warning helps identify the common error of sending the size of the target buffer instead of the size of the data. It does so by detecting when the size used to allocate the buffer is passed, unchanged, to the function putting data in the buffer. + Code analysis name: `BAD_CONCATENATION` ## Example From e67700b8334cd5bff6d07d8c529a302a23ea9cee Mon Sep 17 00:00:00 2001 From: Tyler Whitney Date: Thu, 14 Dec 2023 15:01:36 -0800 Subject: [PATCH 09/10] Update c6059.md small typo --- docs/code-quality/c6059.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index 32e7a46f4a9..d50261ea8b2 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -83,7 +83,7 @@ void f( ) This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. This warning isn't given if some other value is passed as the length parameter, even if that value is incorrect. -Consider the following code, generates warning C6059: +Consider the following code which generates warning C6059: ```cpp #include From 376a2f52879a458fa801a6728a582a36c01c9c33 Mon Sep 17 00:00:00 2001 From: Tyler Whitney Date: Thu, 14 Dec 2023 15:54:52 -0800 Subject: [PATCH 10/10] Update c6059.md acrolinx --- docs/code-quality/c6059.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/code-quality/c6059.md b/docs/code-quality/c6059.md index d50261ea8b2..d78c0f3f74b 100644 --- a/docs/code-quality/c6059.md +++ b/docs/code-quality/c6059.md @@ -83,7 +83,7 @@ void f( ) This analysis detects when the target buffer size is passed unmodified into the length parameter of the string manipulation function. This warning isn't given if some other value is passed as the length parameter, even if that value is incorrect. -Consider the following code which generates warning C6059: +Consider the following code that generates warning C6059: ```cpp #include