Skip to content

[-Wunsafe-buffer-usage] Fix a bug that wrongly assumed CXXMethodDecl always has an identifier #137248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 25, 2025

Conversation

ziqingluo-90
Copy link
Contributor

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed that CXXMethodDecl always has an identifier.

rdar://149071318

…always has an identifier

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed CXXMethodDecl always has an identifier.

rdar://149071318
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:analysis labels Apr 24, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 24, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Ziqing Luo (ziqingluo-90)

Changes

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed that CXXMethodDecl always has an identifier.

rdar://149071318


Full diff: https://github.com/llvm/llvm-project/pull/137248.diff

2 Files Affected:

  • (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+1-1)
  • (added) clang/test/SemaCXX/bug149071318.cpp (+25)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 4eaf8ba61eaec..5b72382ca9772 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -675,7 +675,7 @@ static bool isNullTermPointer(const Expr *Ptr) {
     const CXXMethodDecl *MD = MCE->getMethodDecl();
     const CXXRecordDecl *RD = MCE->getRecordDecl()->getCanonicalDecl();
 
-    if (MD && RD && RD->isInStdNamespace())
+    if (MD && RD && RD->isInStdNamespace() && MD->getIdentifier())
       if (MD->getName() == "c_str" && RD->getName() == "basic_string")
         return true;
   }
diff --git a/clang/test/SemaCXX/bug149071318.cpp b/clang/test/SemaCXX/bug149071318.cpp
new file mode 100644
index 0000000000000..0dbe66f6e37a6
--- /dev/null
+++ b/clang/test/SemaCXX/bug149071318.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:            -verify %s
+
+// This example uncovered a bug in UnsafeBufferUsage.cpp, where the
+// code assumed that a CXXMethodDecl always have an identifier.
+
+int printf( const char* format, char *); // <-- Fake decl of `printf`; to reproduce the bug, this example needs an implicit cast within a printf call.
+
+namespace std { // fake std namespace; to reproduce the bug, a CXXConversionDecl needs to be in std namespace.
+  class X {
+    char * p;
+  public:    
+    operator char*() {return p;}
+  };
+
+  class Y {
+  public:
+    X x;
+  };
+
+}
+
+void test(std::Y &y) {
+  printf("%s", y.x); // expected-warning{{function 'printf' is unsafe}} expected-note{{}}
+}

@ziqingluo-90
Copy link
Contributor Author

CC: @dtarditi

@ziqingluo-90 ziqingluo-90 requested a review from ahatanak April 24, 2025 20:46
@dtarditi
Copy link
Contributor

dtarditi commented Apr 24, 2025

LGTM. Thanks!

Copy link
Contributor

@jkorous-apple jkorous-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}

void test(std::Y &y) {
// Here `y.x` involves an implicit cast and calls the conversion overloading, which has no identifier:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "calls the overloaded cast operator"

@ziqingluo-90 ziqingluo-90 merged commit be48c0d into llvm:main Apr 25, 2025
11 checks passed
@ziqingluo-90 ziqingluo-90 deleted the dev/ziqing/PR-149071318 branch April 25, 2025 00:03
@llvm-ci
Copy link
Collaborator

llvm-ci commented Apr 25, 2025

LLVM Buildbot has detected a new failure on builder lldb-x86_64-debian running on lldb-x86_64-debian while building clang at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/20928

Here is the relevant piece of the build log for the reference
Step 6 (test) failure: build (failure)
...
PASS: lldb-api :: commands/gui/viewlarge/TestGuiViewLarge.py (30 of 2831)
PASS: lldb-api :: functionalities/load_unload/TestLoadUnload.py (31 of 2831)
PASS: lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests/8/35 (32 of 2831)
PASS: lldb-api :: commands/expression/expr-in-syscall/TestExpressionInSyscall.py (33 of 2831)
PASS: lldb-api :: functionalities/recursion/TestValueObjectRecursion.py (34 of 2831)
PASS: lldb-api :: commands/gui/spawn-threads/TestGuiSpawnThreads.py (35 of 2831)
PASS: lldb-api :: commands/watchpoints/hello_watchlocation/TestWatchLocation.py (36 of 2831)
PASS: lldb-api :: tools/lldb-dap/variables/TestDAP_variables.py (37 of 2831)
PASS: lldb-api :: functionalities/gdb_remote_client/TestRegDefinitionInParts.py (38 of 2831)
PASS: lldb-api :: functionalities/gdb_remote_client/TestXMLRegisterFlags.py (39 of 2831)
FAIL: lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py (40 of 2831)
******************** TEST 'lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py' FAILED ********************
Script:
--
/usr/bin/python3 /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./lib --env LLVM_INCLUDE_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/include --env LLVM_TOOLS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./bin --arch x86_64 --build-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex --lldb-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/lldb --compiler /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/clang --dsymutil /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/dsymutil --make /usr/bin/gmake --llvm-tools-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./bin --lldb-obj-root /home/worker/2.0.1/lldb-x86_64-debian/build/tools/lldb --lldb-libs-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./lib -t /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/tools/lldb-dap/launch -p TestDAP_launch.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 21.0.0git (https://github.com/llvm/llvm-project.git revision be48c0df77413a237565a339c9ccc275b8256631)
  clang revision be48c0df77413a237565a339c9ccc275b8256631
  llvm revision be48c0df77413a237565a339c9ccc275b8256631
Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
Change dir to: /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/tools/lldb-dap/launch
runCmd: settings clear -all

output: 

runCmd: settings set symbols.enable-external-lookup false

output: 

runCmd: settings set target.inherit-tcc true

output: 

runCmd: settings set target.disable-aslr false

output: 

runCmd: settings set target.detach-on-error false

output: 


ziqingluo-90 added a commit to swiftlang/llvm-project that referenced this pull request May 5, 2025
…always has an identifier (llvm#137248)

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed that
CXXMethodDecl always has an identifier.

rdar://149071318
(cherry picked from commit be48c0d)
ziqingluo-90 added a commit to swiftlang/llvm-project that referenced this pull request May 6, 2025
…always has an identifier (llvm#137248)

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed that
CXXMethodDecl always has an identifier.

rdar://149071318
(cherry picked from commit be48c0d)
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…always has an identifier (llvm#137248)

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed that
CXXMethodDecl always has an identifier.

rdar://149071318
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…always has an identifier (llvm#137248)

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed that
CXXMethodDecl always has an identifier.

rdar://149071318
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…always has an identifier (llvm#137248)

Fix a bug in UnsafeBufferUsage.cpp that wrongly assumed that
CXXMethodDecl always has an identifier.

rdar://149071318
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants