From 5aa3cf3a0bacfb33293ecda58236e85f73b796ea Mon Sep 17 00:00:00 2001 From: flovent Date: Sat, 23 Aug 2025 13:47:55 +0800 Subject: [PATCH 1/8] Fix #7570 support address-of operator on variables in `getBufferSize()` --- lib/checkbufferoverrun.cpp | 4 +++- test/testbufferoverrun.cpp | 31 ++++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index a43d5b26ee8..d7cdd8ab1d2 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -553,6 +553,8 @@ ValueFlow::Value CheckBufferOverrun::getBufferSize(const Token *bufTok) const { if (!bufTok->valueType()) return ValueFlow::Value(-1); + if (bufTok->isUnaryOp("&")) + bufTok = bufTok->astOperand1(); const Variable *var = bufTok->variable(); if (!var || var->dimensions().empty()) { @@ -653,7 +655,7 @@ void CheckBufferOverrun::bufferOverflow() argtok = argtok->astOperand2() ? argtok->astOperand2() : argtok->astOperand1(); while (Token::Match(argtok, ".|::")) argtok = argtok->astOperand2(); - if (!argtok || !argtok->variable()) + if (!argtok) continue; if (argtok->valueType() && argtok->valueType()->pointer == 0) continue; diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 16f0f4c3f72..67d5c83683b 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -306,6 +306,8 @@ class TestBufferOverrun : public TestFixture { TEST_CASE(objectIndex); TEST_CASE(checkPipeParameterSize); // ticket #3521 + + TEST_CASE(getBufferSizeOfAddressOfVariable); // ticket #7570 } @@ -4320,7 +4322,7 @@ class TestBufferOverrun : public TestFixture { " char c;\n" " mymemset(&c, 0, 4);\n" "}", dinit(CheckOptions, $.s = &settings)); - TODO_ASSERT_EQUALS("[test.cpp:3:14]: (error) Buffer is accessed out of bounds: c [bufferAccessOutOfBounds]\n", "", errout_str()); + ASSERT_EQUALS("[test.cpp:3:12]: (error) Buffer is accessed out of bounds: &c [bufferAccessOutOfBounds]\n", errout_str()); // ticket #2121 - buffer access out of bounds when using uint32_t check("void f(void) {\n" @@ -5712,6 +5714,33 @@ class TestBufferOverrun : public TestFixture { "}", dinit(CheckOptions, $.s = &settings)); ASSERT_EQUALS("", errout_str()); } + + void getBufferSizeOfAddressOfVariable() { // #7570 + + check("void f() {\n" + " int i;\n" + " memset(&i, 0, 1000);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); + + check("void f() {\n" + " int i;\n" + " memset(&i, 0, sizeof(i));\n" + "}"); + ASSERT_EQUALS("", errout_str()); + + check("void foo() {\n" + " char c[6];\n" + " strncpy(&c, \"hello!\", 6);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:3]: (warning, inconclusive) The buffer '&c' may not be null-terminated after the call to strncpy(). [terminateStrncpy]\n", errout_str()); + + check("void foo() {\n" + " char c[6];\n" + " memcpy(&c, \"hello\\n\", 6);\n" + "}"); + ASSERT_EQUALS("", errout_str()); + } }; REGISTER_TEST(TestBufferOverrun) From 70c1ddd75ae6eaa1aaa7b8cd658c5b9b608d5b01 Mon Sep 17 00:00:00 2001 From: flovent Date: Sat, 23 Aug 2025 18:54:12 +0800 Subject: [PATCH 2/8] [NFC] Format test file --- test/testbufferoverrun.cpp | 40 +++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 67d5c83683b..ae02e086e2e 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -5717,29 +5717,29 @@ class TestBufferOverrun : public TestFixture { void getBufferSizeOfAddressOfVariable() { // #7570 - check("void f() {\n" - " int i;\n" - " memset(&i, 0, 1000);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); + check("void f() {\n" + " int i;\n" + " memset(&i, 0, 1000);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); - check("void f() {\n" - " int i;\n" - " memset(&i, 0, sizeof(i));\n" - "}"); - ASSERT_EQUALS("", errout_str()); + check("void f() {\n" + " int i;\n" + " memset(&i, 0, sizeof(i));\n" + "}"); + ASSERT_EQUALS("", errout_str()); - check("void foo() {\n" - " char c[6];\n" - " strncpy(&c, \"hello!\", 6);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3:3]: (warning, inconclusive) The buffer '&c' may not be null-terminated after the call to strncpy(). [terminateStrncpy]\n", errout_str()); + check("void foo() {\n" + " char c[6];\n" + " strncpy(&c, \"hello!\", 6);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:3]: (warning, inconclusive) The buffer '&c' may not be null-terminated after the call to strncpy(). [terminateStrncpy]\n", errout_str()); - check("void foo() {\n" - " char c[6];\n" - " memcpy(&c, \"hello\\n\", 6);\n" - "}"); - ASSERT_EQUALS("", errout_str()); + check("void foo() {\n" + " char c[6];\n" + " memcpy(&c, \"hello\\n\", 6);\n" + "}"); + ASSERT_EQUALS("", errout_str()); } }; From ed53e76c2d2a8d1fb7e9cf48a5ec14ab5a979ac8 Mon Sep 17 00:00:00 2001 From: flovent Date: Sat, 23 Aug 2025 19:05:49 +0800 Subject: [PATCH 3/8] Improve error message: remove `&` when operand is bufer --- lib/checkbufferoverrun.cpp | 12 ++++++++++-- test/testbufferoverrun.cpp | 8 +++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index d7cdd8ab1d2..ae9638af0ac 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -75,6 +75,14 @@ static const ValueFlow::Value *getBufferSizeValue(const Token *tok) return it == tokenValues.cend() ? nullptr : &*it; } +static const Token* getRealBufferTok(const Token* tok) { + if (!tok->isUnaryOp("&")) + return tok; + + const auto* op = tok->astOperand1(); + return (op->valueType() && op->valueType()->pointer) ? op : tok; +} + static int getMinFormatStringOutputLength(const std::vector ¶meters, nonneg int formatStringArgNr) { if (formatStringArgNr <= 0 || formatStringArgNr > parameters.size()) @@ -690,7 +698,7 @@ void CheckBufferOverrun::bufferOverflow() void CheckBufferOverrun::bufferOverflowError(const Token *tok, const ValueFlow::Value *value, Certainty certainty) { - reportError(getErrorPath(tok, value, "Buffer overrun"), Severity::error, "bufferAccessOutOfBounds", "Buffer is accessed out of bounds: " + (tok ? tok->expressionString() : "buf"), CWE_BUFFER_OVERRUN, certainty); + reportError(getErrorPath(tok, value, "Buffer overrun"), Severity::error, "bufferAccessOutOfBounds", "Buffer is accessed out of bounds: " + (tok ? getRealBufferTok(tok)->expressionString() : "buf"), CWE_BUFFER_OVERRUN, certainty); } //--------------------------------------------------------------------------- @@ -800,7 +808,7 @@ void CheckBufferOverrun::stringNotZeroTerminated() if (isZeroTerminated) continue; // TODO: Locate unsafe string usage.. - terminateStrncpyError(tok, args[0]->expressionString()); + terminateStrncpyError(tok, getRealBufferTok(args[0])->expressionString()); } } } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index ae02e086e2e..baa9d26d41d 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -5723,6 +5723,12 @@ class TestBufferOverrun : public TestFixture { "}"); ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); + check("void f() {\n" + " int i[2];\n" + " memset(&i, 0, 1000);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: i [bufferAccessOutOfBounds]\n", errout_str()); + check("void f() {\n" " int i;\n" " memset(&i, 0, sizeof(i));\n" @@ -5733,7 +5739,7 @@ class TestBufferOverrun : public TestFixture { " char c[6];\n" " strncpy(&c, \"hello!\", 6);\n" "}"); - ASSERT_EQUALS("[test.cpp:3:3]: (warning, inconclusive) The buffer '&c' may not be null-terminated after the call to strncpy(). [terminateStrncpy]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:3:3]: (warning, inconclusive) The buffer 'c' may not be null-terminated after the call to strncpy(). [terminateStrncpy]\n", errout_str()); check("void foo() {\n" " char c[6];\n" From 08b481bf63274e95a84ca6f15f2c585568a0773d Mon Sep 17 00:00:00 2001 From: flovent Date: Sat, 23 Aug 2025 19:10:27 +0800 Subject: [PATCH 4/8] [NFC] Add TODO testcase for `&buf[i]` --- test/testbufferoverrun.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index baa9d26d41d..650b538709a 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -5735,6 +5735,12 @@ class TestBufferOverrun : public TestFixture { "}"); ASSERT_EQUALS("", errout_str()); + check("void f() {\n" + " int i[10];\n" + " memset(&i[1], 0, 1000);\n" + "}"); + TODO_ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i[1] [bufferAccessOutOfBounds]\n", "", errout_str()); + check("void foo() {\n" " char c[6];\n" " strncpy(&c, \"hello!\", 6);\n" From a46e4aa2c0dd5cb670b972df26488003fb94e280 Mon Sep 17 00:00:00 2001 From: flovent Date: Sat, 23 Aug 2025 19:36:49 +0800 Subject: [PATCH 5/8] Fix a wrong testcase --- test/testbufferoverrun.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 650b538709a..9aac4bd785a 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -5749,7 +5749,7 @@ class TestBufferOverrun : public TestFixture { check("void foo() {\n" " char c[6];\n" - " memcpy(&c, \"hello\\n\", 6);\n" + " strncpy(&c, \"hello\\0\", 6);\n" "}"); ASSERT_EQUALS("", errout_str()); } From 8f99d67b137109c76b491d7bd998a6e746d6fdd5 Mon Sep 17 00:00:00 2001 From: flovent Date: Wed, 27 Aug 2025 15:23:03 +0800 Subject: [PATCH 6/8] [NFC] Address comment: `auto` to `Token` --- lib/checkbufferoverrun.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index ae9638af0ac..01d8be4c617 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -79,7 +79,7 @@ static const Token* getRealBufferTok(const Token* tok) { if (!tok->isUnaryOp("&")) return tok; - const auto* op = tok->astOperand1(); + const Token* op = tok->astOperand1(); return (op->valueType() && op->valueType()->pointer) ? op : tok; } From 52253a506b1ef71962964445d9c9d75b8f13c450 Mon Sep 17 00:00:00 2001 From: flovent Date: Thu, 11 Sep 2025 20:21:00 +0800 Subject: [PATCH 7/8] [NFC] Add testcase requested and organize test cases --- test/testbufferoverrun.cpp | 89 ++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 41 deletions(-) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 9aac4bd785a..135c68c0409 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -231,6 +231,7 @@ class TestBufferOverrun : public TestFixture { TEST_CASE(buffer_overrun_function_array_argument); TEST_CASE(possible_buffer_overrun_1); // #3035 TEST_CASE(buffer_overrun_readSizeFromCfg); + TEST_CASE(buffer_overrun_handle_addr_of_var); // ticket #7570 - correctly handle &var TEST_CASE(valueflow_string); // using ValueFlow string values in checking @@ -270,6 +271,7 @@ class TestBufferOverrun : public TestFixture { TEST_CASE(terminateStrncpy3); TEST_CASE(terminateStrncpy4); TEST_CASE(terminateStrncpy5); // #9944 + TEST_CASE(terminateStrncpy_handle_addr_of_var); // #7570 TEST_CASE(recursive_long_time); TEST_CASE(crash1); // Ticket #1587 - crash @@ -306,8 +308,6 @@ class TestBufferOverrun : public TestFixture { TEST_CASE(objectIndex); TEST_CASE(checkPipeParameterSize); // ticket #3521 - - TEST_CASE(getBufferSizeOfAddressOfVariable); // ticket #7570 } @@ -3725,6 +3725,38 @@ class TestBufferOverrun : public TestFixture { ASSERT_EQUALS("[test.cpp:3:16]: (error) Buffer is accessed out of bounds: ms.str [bufferAccessOutOfBounds]\n", errout_str()); } + void buffer_overrun_handle_addr_of_var() { // #7570 + check("void f() {\n" + " int i;\n" + " memset(i, 0, 1000);\n" + "}"); + ASSERT_EQUALS("", errout_str()); + + check("void f() {\n" + " int i;\n" + " memset(&i, 0, 1000);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); + + check("void f() {\n" + " int i[2];\n" + " memset(&i, 0, 1000);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: i [bufferAccessOutOfBounds]\n", errout_str()); + + check("void f() {\n" + " int i;\n" + " memset(&i, 0, sizeof(i));\n" + "}"); + ASSERT_EQUALS("", errout_str()); + + check("void f() {\n" + " int i[10];\n" + " memset(&i[1], 0, 1000);\n" + "}"); + TODO_ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i[1] [bufferAccessOutOfBounds]\n", "", errout_str()); + } + void valueflow_string() { // using ValueFlow string values in checking check("char f() {\n" " const char *x = s;\n" @@ -4686,6 +4718,20 @@ class TestBufferOverrun : public TestFixture { } // extracttests.enable + void terminateStrncpy_handle_addr_of_var() { // #7570 + check("void foo() {\n" + " char c[6];\n" + " strncpy(&c, \"hello!\", 6);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3:3]: (warning, inconclusive) The buffer 'c' may not be null-terminated after the call to strncpy(). [terminateStrncpy]\n", errout_str()); + + check("void foo() {\n" + " char c[6];\n" + " strncpy(&c, \"hello\\0\", 6);\n" + "}"); + ASSERT_EQUALS("", errout_str()); + } + void recursive_long_time() { // Just test that recursive check doesn't take long time check("char *f2 ( char *b )\n" @@ -5714,45 +5760,6 @@ class TestBufferOverrun : public TestFixture { "}", dinit(CheckOptions, $.s = &settings)); ASSERT_EQUALS("", errout_str()); } - - void getBufferSizeOfAddressOfVariable() { // #7570 - - check("void f() {\n" - " int i;\n" - " memset(&i, 0, 1000);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); - - check("void f() {\n" - " int i[2];\n" - " memset(&i, 0, 1000);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: i [bufferAccessOutOfBounds]\n", errout_str()); - - check("void f() {\n" - " int i;\n" - " memset(&i, 0, sizeof(i));\n" - "}"); - ASSERT_EQUALS("", errout_str()); - - check("void f() {\n" - " int i[10];\n" - " memset(&i[1], 0, 1000);\n" - "}"); - TODO_ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i[1] [bufferAccessOutOfBounds]\n", "", errout_str()); - - check("void foo() {\n" - " char c[6];\n" - " strncpy(&c, \"hello!\", 6);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3:3]: (warning, inconclusive) The buffer 'c' may not be null-terminated after the call to strncpy(). [terminateStrncpy]\n", errout_str()); - - check("void foo() {\n" - " char c[6];\n" - " strncpy(&c, \"hello\\0\", 6);\n" - "}"); - ASSERT_EQUALS("", errout_str()); - } }; REGISTER_TEST(TestBufferOverrun) From fad2b9b7a4b49fc7c5e17408f9a59d5be5f87e8e Mon Sep 17 00:00:00 2001 From: flovent Date: Thu, 11 Sep 2025 21:01:56 +0800 Subject: [PATCH 8/8] Fix formatting --- test/testbufferoverrun.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 135c68c0409..7cda55bd1fb 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -3726,35 +3726,35 @@ class TestBufferOverrun : public TestFixture { } void buffer_overrun_handle_addr_of_var() { // #7570 - check("void f() {\n" + check("void f() {\n" " int i;\n" " memset(i, 0, 1000);\n" "}"); - ASSERT_EQUALS("", errout_str()); + ASSERT_EQUALS("", errout_str()); - check("void f() {\n" + check("void f() {\n" " int i;\n" " memset(&i, 0, 1000);\n" "}"); - ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i [bufferAccessOutOfBounds]\n", errout_str()); - check("void f() {\n" + check("void f() {\n" " int i[2];\n" " memset(&i, 0, 1000);\n" "}"); - ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: i [bufferAccessOutOfBounds]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: i [bufferAccessOutOfBounds]\n", errout_str()); - check("void f() {\n" + check("void f() {\n" " int i;\n" " memset(&i, 0, sizeof(i));\n" "}"); - ASSERT_EQUALS("", errout_str()); + ASSERT_EQUALS("", errout_str()); - check("void f() {\n" + check("void f() {\n" " int i[10];\n" " memset(&i[1], 0, 1000);\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i[1] [bufferAccessOutOfBounds]\n", "", errout_str()); + TODO_ASSERT_EQUALS("[test.cpp:3:10]: (error) Buffer is accessed out of bounds: &i[1] [bufferAccessOutOfBounds]\n", "", errout_str()); } void valueflow_string() { // using ValueFlow string values in checking