Skip to content

Comments

Fix various errors found with UBSan#8184

Open
damyanp wants to merge 8 commits intomicrosoft:mainfrom
damyanp:ub
Open

Fix various errors found with UBSan#8184
damyanp wants to merge 8 commits intomicrosoft:mainfrom
damyanp:ub

Conversation

@damyanp
Copy link
Member

@damyanp damyanp commented Feb 20, 2026

  • SPIRVOptions.h - one of the fields of this struct (signaturePacking) wasn't getting initialized before the struct's assignment operator was used. This resulted in the uninitialized fields being copied, which is UB. Although a more surgical fix is possible, it seemed better to just ensure all fields are initialized, setting a good precedent if any new fields are added.
  • FrontendOptions.h - uninitialized field
  • APInt.cpp - left shift of a signed integer
  • Triple.h - uninitialized field
  • VirtualFileSystem.h - uninitialized field
  • Lookup.h - uninitialized field
  • MemoryBuffer.cpp - avoid memcpy from a null dest (UB even if size is 0)

Methodology: had copilot CLI get a baseline running the tests under ubsan, categorize the failures and fix them, and verify that gcc-14 and clang 18 are now ubsan clan.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

damyanp and others added 7 commits February 20, 2026 03:52
All bool, uint32_t, and enum members in SpirvCodeGenOptions were left
uninitialized by the implicit default constructor. Reading these
indeterminate values is undefined behavior. Add in-class member
initializers for all scalar fields.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The default constructor omitted IsSystem from its initializer list,
leaving it indeterminate. Reading it is undefined behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Left-shifting a negative signed integer is UB before C++20. Cast to
uint64_t for the left shift, then back to int64_t for the arithmetic
right shift that performs sign extension.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The default constructor only initialized Type, leaving Perms,
IsVFSMapped, User, Group, and Size indeterminate.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The copy constructor reads the Ambiguity field even when ResultKind
is not Ambiguous, triggering a load of an indeterminate enum value.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SubArch was left uninitialized for architectures without a
sub-architecture, causing loads of indeterminate enum values.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Passing a null pointer to memcpy is UB even when size is 0. Guard
the call with a size check for empty StringRef inputs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 02686effda507a065d729f402567af76b52485f4 26389eb8e88b966d3d6be13851069ef9dd599ea2 -- include/dxc/Support/SPIRVOptions.h include/llvm/ADT/Triple.h lib/Support/APInt.cpp lib/Support/MemoryBuffer.cpp tools/clang/include/clang/Basic/VirtualFileSystem.h tools/clang/include/clang/Frontend/FrontendOptions.h tools/clang/include/clang/Sema/Lookup.h
View the diff from clang-format here.
diff --git a/include/dxc/Support/SPIRVOptions.h b/include/dxc/Support/SPIRVOptions.h
index ede9fc0f..f39602e9 100644
--- a/include/dxc/Support/SPIRVOptions.h
+++ b/include/dxc/Support/SPIRVOptions.h
@@ -109,9 +109,11 @@ struct SpirvCodeGenOptions {
   std::optional<BindingInfo> samplerHeapBinding;
   std::optional<BindingInfo> counterHeapBinding;
 
-  bool signaturePacking = false; ///< Whether signature packing is enabled or not
+  bool signaturePacking =
+      false; ///< Whether signature packing is enabled or not
 
-  bool printAll = false; // Dump SPIR-V module before each pass and after the last one.
+  bool printAll =
+      false; // Dump SPIR-V module before each pass and after the last one.
 
   // String representation of all command line options and input file.
   std::string clOptions;
diff --git a/lib/Support/APInt.cpp b/lib/Support/APInt.cpp
index 8b80b7ef..df41e00e 100644
--- a/lib/Support/APInt.cpp
+++ b/lib/Support/APInt.cpp
@@ -552,8 +552,10 @@ bool APInt::ult(const APInt& RHS) const {
 bool APInt::slt(const APInt& RHS) const {
   assert(BitWidth == RHS.BitWidth && "Bit widths must be same for comparison");
   if (isSingleWord()) {
-    int64_t lhsSext = int64_t(uint64_t(VAL) << (64-BitWidth)) >> (64-BitWidth);
-    int64_t rhsSext = int64_t(uint64_t(RHS.VAL) << (64-BitWidth)) >> (64-BitWidth);
+    int64_t lhsSext =
+        int64_t(uint64_t(VAL) << (64 - BitWidth)) >> (64 - BitWidth);
+    int64_t rhsSext =
+        int64_t(uint64_t(RHS.VAL) << (64 - BitWidth)) >> (64 - BitWidth);
     return lhsSext < rhsSext;
   }
 
@@ -1061,8 +1063,8 @@ APInt APInt::ashr(unsigned shiftAmt) const {
       return APInt(BitWidth, 0); // undefined
     else {
       unsigned SignBit = APINT_BITS_PER_WORD - BitWidth;
-      return APInt(BitWidth,
-        (((int64_t(uint64_t(VAL) << SignBit) >> SignBit) >> shiftAmt)));
+      return APInt(BitWidth, (((int64_t(uint64_t(VAL) << SignBit) >> SignBit) >>
+                               shiftAmt)));
     }
   }
 
diff --git a/lib/Support/MemoryBuffer.cpp b/lib/Support/MemoryBuffer.cpp
index e28144ff..608bda74 100644
--- a/lib/Support/MemoryBuffer.cpp
+++ b/lib/Support/MemoryBuffer.cpp
@@ -127,7 +127,7 @@ MemoryBuffer::getMemBufferCopy(StringRef InputData, const Twine &BufferName) {
   if (!Buf)
     return nullptr;
   if (InputData.size())
-    memcpy(const_cast<char*>(Buf->getBufferStart()), InputData.data(),
+    memcpy(const_cast<char *>(Buf->getBufferStart()), InputData.data(),
            InputData.size());
   return Buf;
 }
diff --git a/tools/clang/include/clang/Frontend/FrontendOptions.h b/tools/clang/include/clang/Frontend/FrontendOptions.h
index ae83c231..dabf1a4b 100644
--- a/tools/clang/include/clang/Frontend/FrontendOptions.h
+++ b/tools/clang/include/clang/Frontend/FrontendOptions.h
@@ -92,7 +92,7 @@ class FrontendInputFile {
   bool IsSystem;
 
 public:
-  FrontendInputFile() : Buffer(nullptr), Kind(IK_None), IsSystem(false) { }
+  FrontendInputFile() : Buffer(nullptr), Kind(IK_None), IsSystem(false) {}
   FrontendInputFile(StringRef File, InputKind Kind, bool IsSystem = false)
     : File(File.str()), Buffer(nullptr), Kind(Kind), IsSystem(IsSystem) { }
   FrontendInputFile(llvm::MemoryBuffer *buffer, InputKind Kind,
  • Check this box to apply formatting changes to this branch.

@damyanp damyanp marked this pull request as ready for review February 20, 2026 20:25
Copy link
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

2 participants