[ASan] Correctly handle vectorized pointer sub/cmp for `invalid-pointer-pair`

Project: llvm/llvm-project | Language: C++ | Concepts: refactoring, error-handling | Difficulty: mid
Link: https://github.com/llvm/llvm-project/pull/213546

The problem

AddressSanitizer (ASan) did not correctly handle pointer subtraction or comparison when the operands were vectors of pointers. ASan would pass the entire vector value to runtime functions like __sanitizer_ptr_sub or __sanitizer_ptr_cmp, which expect scalar (single) pointer arguments. This caused assertion failures at runtime due to type mismatches.

Why it's non-trivial

A naive fix might simply try to cast the vector to an integer type or ignore vector types altogether. However, this would not properly check each element of the vector for invalid pointer pairs, and would not provide the necessary runtime instrumentation for each pointer in the vector. The solution must handle each element of the vector individually, preserving correct instrumentation and error reporting.

The approach

The main change is in the AddressSanitizer::instrumentPointerComparisonOrSubtraction function. The new logic checks if the operands are vectors, and if so, extracts and instruments each element separately.

1. Change the function to return bool to indicate if instrumentation occurred:

-  void instrumentPointerComparisonOrSubtraction(Instruction *I,
+  bool instrumentPointerComparisonOrSubtraction(Instruction *I,
                                                 RuntimeCallInserter &RTCI);

This allows the caller to know if the function made any changes, which is important for tracking whether the function was modified.

2. Detect vector types and handle them elementwise:

+  if (const auto *Ty = Param[0]->getType(); Ty->isVectorTy()) {
+    const auto *VTy = dyn_cast<FixedVectorType>(Ty);
+    // Skip scalable vectors
+    if (!VTy)
+      return false;
+
+    assert(Param[1]->getType()->isVectorTy() &&
+           VTy->getElementCount() ==
+               cast<VectorType>(Param[1]->getType())->getElementCount() &&
+           "invalid vector pointer pair instrumentation operands");
+    for (unsigned Index = 0, NumElements = VTy->getNumElements();
+         Index != NumElements; ++Index) {
+      Value *ScalarParam[2] = {
+          IRB.CreateExtractElement(Param[0], IRB.getInt32(Index)),
+          IRB.CreateExtractElement(Param[1], IRB.getInt32(Index))};
+      for (Value *&P : ScalarParam)
+        P = IRB.CreatePointerCast(P, IntptrTy);
+      RTCI.createRuntimeCall(IRB, F, ScalarParam);
+    }
+    return true;
+  }

Checks if the operand is a vector type.
Skips scalable vectors (not supported).
Asserts that both operands are vectors of the same length.
For each element in the vector, extracts the element, casts it to the appropriate type, and emits a runtime call for that pair.

3. Fallback for scalar pointers (original logic):

   for (Value *&P : Param)
     P = IRB.CreatePointerCast(P, IntptrTy);
   RTCI.createRuntimeCall(IRB, F, Param);
+  return true;

For non-vector operands, simply cast and instrument as before.

4. Update the call site to use the new return value:

-    instrumentPointerComparisonOrSubtraction(Inst, RTCI);
-    FunctionModified = true;
+    FunctionModified |= instrumentPointerComparisonOrSubtraction(Inst, RTCI);

Ensures that the function modification status is correctly tracked.

5. Add tests to verify vector pointer pair instrumentation:

New C++ test:

+int main() {
+  const char* begins[2] = {"abc", "def"};
+  const char* ends[2] = {"abcde", "defgh"};
+  long lengths[2] {0};
+  lengths[0] = ends[0] - begins[0];
+  lengths[1] = ends[1] - begins[1];
+  return 0;
+}

LLVM IR test checks that the sanitizer emits a call for each vector element:

+define <2 x i64> @mysub_vector(<2 x ptr> %p, <2 x ptr> %q) sanitize_address {
+; SUB: [[P0:%[0-9A-Za-z]+]] = extractelement <2 x i64> %x, i32 0
+; SUB: [[Q0:%[0-9A-Za-z]+]] = extractelement <2 x i64> %y, i32 0
+; SUB: call void @__sanitizer_ptr_sub(i64 [[P0]], i64 [[Q0]])
+; SUB: [[P1:%[0-9A-Za-z]+]] = extractelement <2 x i64> %x, i32 1
+; SUB: [[Q1:%[0-9A-Za-z]+]] = extractelement <2 x i64> %y, i32 1
+; SUB: call void @__sanitizer_ptr_sub(i64 [[P1]], i64 [[Q1]])
+  %z = sub <2 x i64> %x, %y
+  ret <2 x i64> %z
+}

From the review thread

thurstond: Please upload a separate PR with a crashing test (with // XFAIL: * and // REQUIRES: asserts) to demonstrate the issue (this fix PR would then remove the XFAIL, proving the fix works).

FWIW even without this fix, this test case did not crash for me. AFAICS from -mllvm -print-before=asan, this test case is too easy, and the compiler had optimized away the entire main function into ret i32 0 (https://godbolt.org/z/hhvv35TPr). OTOH I could reproduce the crash from https://github.com/llvm/llvm-project/issues/212453: https:/

flovent: Thanks for the detailed guidance and followup fix for testcase!

Takeaways

When instrumenting or transforming code, always handle vector types explicitly—scalar logic often does not generalize.
For runtime instrumentation, ensure that each element of a vector is checked individually if the runtime expects scalar arguments.
When making an instrumentation pass, track whether the function was modified to avoid unnecessary recompilation or missed updates.

[ASan] Correctly handle vectorized pointer sub/cmp for invalid-pointer-pair