Skip to content
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

Reland "[libclang] Always Dup in createRef(StringRef)" #127078

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Feb 13, 2025

Reverts #127076 to reland #125020

Use-after-free should be fixed here #127063

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:as-a-library libclang and C++ API labels Feb 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 13, 2025

@llvm/pr-subscribers-clang

Author: Vitaly Buka (vitalybuka)

Changes

Reverts llvm/llvm-project#127076


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

2 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/tools/libclang/CXString.cpp (+1-13)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 03bddbe3e983a..e41ad384b84f7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -257,6 +257,9 @@ clang-format
 libclang
 --------
 
+- Fixed a buffer overflow in ``CXString`` implementation. The fix may result in
+  increased memory allocation.
+
 Code Completion
 ---------------
 
diff --git a/clang/tools/libclang/CXString.cpp b/clang/tools/libclang/CXString.cpp
index 5e427957a1092..aaa8f8eeb67a1 100644
--- a/clang/tools/libclang/CXString.cpp
+++ b/clang/tools/libclang/CXString.cpp
@@ -87,19 +87,7 @@ CXString createRef(StringRef String) {
   if (String.empty())
     return createEmpty();
 
-  // If the string is not nul-terminated, we have to make a copy.
-
-  // FIXME: This is doing a one past end read, and should be removed! For memory
-  // we don't manage, the API string can become unterminated at any time outside
-  // our control.
-
-  if (String.data()[String.size()] != 0)
-    return createDup(String);
-
-  CXString Result;
-  Result.data = String.data();
-  Result.private_flags = (unsigned) CXS_Unmanaged;
-  return Result;
+  return createDup(String);
 }
 
 CXString createDup(StringRef String) {

@vitalybuka vitalybuka marked this pull request as draft February 13, 2025 15:43
@vitalybuka vitalybuka marked this pull request as ready for review February 19, 2025 06:43
@vitalybuka vitalybuka added the skip-precommit-approval PR for CI feedback, not intended for review label Feb 19, 2025
Created using spr 1.3.4
@vitalybuka
Copy link
Collaborator Author

@vitalybuka vitalybuka merged commit a16fa3a into main Feb 20, 2025
11 checks passed
@vitalybuka vitalybuka deleted the revert-127076-revert-125020-users/vitalybuka/spr/libclang-always-dup-in-createref branch February 20, 2025 02:41
@vitalybuka
Copy link
Collaborator Author

@fmayer Looks like my patch exposed another UAF
https://lab.llvm.org/buildbot/#/builders/169/builds/8637

Looking, and will revert if there is no quick fix

vitalybuka added a commit that referenced this pull request Feb 20, 2025
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 27, 2025
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:as-a-library libclang and C++ API clang Clang issues not falling into any other category skip-precommit-approval PR for CI feedback, not intended for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants