[SCCP] Update condition to avoid overflow.

[asbirlea]

Summary

Update condition to remove addition that may cause an overflow.
Resolves PR42814.

[lebedev.ri]

lib/Analysis/ConstantFolding.cpp

550–552 ↗

(On Diff #212220)

Is Offset==0 == 0'th byte of the constant?
If so, perhaps this check could simply be done first?

[asbirlea]

Update per comment suggestion: reorder conditions.

[lebedev.ri]

lib/Analysis/ConstantFolding.cpp

551 ↗

(On Diff #212226)

This check looks suspicious to me.
I'd expect this to be if (Offset + BytesLoaded >= InitializerSize),
this way we are checking that BytesLoaded bytes lies within the global.

[asbirlea]

lib/Analysis/ConstantFolding.cpp

551 ↗

(On Diff #212226)

If Offset can be a negative value, it's possible to get an OOB access when Offset + BytesLoaded <= 0.
We can also get an OOB access if (Offset + BytesLoaded >= InitializerSize), but the two checks seem orthogonal given this section of code (I'm not familiar with the larger scope of this code).

[lebedev.ri]

lib/Analysis/ConstantFolding.cpp

551 ↗

(On Diff #212226)

Okay, sounds plausible.

[fhahn]

lib/Analysis/ConstantFolding.cpp

551 ↗

(On Diff #212226)

I think with the check as it is, there still could be an overflow *in theory*, e.g. for constants that are int64_max big. Given that we only allow global constants with initializers, this would mean a huge bitcode file, but while you're at it, it might be worth changing the comparisons to if (Offfset <= -BytesLoaded) as well

[RKSimon]

test/Transforms/SCCP/ubsan_overflow.ll

1 ↗

(On Diff #212226)

Shouldn't this be

opt < %s -sccp -S

[asbirlea]

Address comment.

test/Transforms/SCCP/ubsan_overflow.ll

1 ↗

(On Diff #212226)

The behavior is the same.

[asbirlea]

Added needed cast from unsigned to int64_t.

[sanjoy.google]

lib/Analysis/ConstantFolding.cpp

551 ↗

(On Diff #212428)

Use static_cast? That also makes it obvious whether the cast happens before or after the multiply.

[asbirlea]

Address comment.

[sanjoy.google]

lgtm with some nits

lib/Analysis/ConstantFolding.cpp

547 ↗

(On Diff #212609)

OOC is there a reason to flip the order of the two checks? If yes, can you please update the commit message (so as to not confuse folks digging through the commit history in the future)?

test/Transforms/SCCP/ubsan_overflow.ll

12 ↗

(On Diff #212609)

We probably should remove the unreachable. As written the function has unconditional UB which means some other optimization within SCCP could kick in.

Probably the best thing to do is to return the value you loaded so that it can't get DCE'd away either (SCCP is unlikely to do that, but you never know).

[asbirlea]

Address comments.
Thank you for the review!

llvm/trunk/lib/Analysis/ConstantFolding.cpp

Constant *FoldReinterpretLoadFromConstPtr(Constant *C, Type *LoadTy,
   if (!GV || !GV->isConstant() || !GV->hasDefinitiveInitializer() ||
       !GV->getInitializer()->getType()->isSized())
     return nullptr;
 
   int64_t Offset = OffsetAI.getSExtValue();
   int64_t InitializerSize = DL.getTypeAllocSize(GV->getInitializer()->getType());
 
   // If we're not accessing anything in this constant, the result is undefined.
-  if (Offset + BytesLoaded <= 0)
+  if (Offset <= -1 * static_cast<int64_t>(BytesLoaded))
     return UndefValue::get(IntType);
 
   // If we're not accessing anything in this constant, the result is undefined.
   if (Offset >= InitializerSize)
     return UndefValue::get(IntType);
 
   unsigned char RawBytes[32] = {0};
   unsigned char *CurPtr = RawBytes;

llvm/trunk/test/Transforms/SCCP/ubsan_overflow.ll

+; RUN: opt -sccp -S %s | FileCheck %s
+
+@0 = private unnamed_addr constant [16 x i8] c"\01\00\00\00\01\01\00\00\01\01\01\00\01\01\01\01"
+
+; CHECK-LABEL: @foo
+define i8 @foo() {
+entry:
+  %0 = add nuw nsw i64 0, -1
+  %1 = lshr i64 %0, 1
+  %2 = getelementptr inbounds [4 x [4 x i8]], [4 x [4 x i8]]* bitcast ([16 x i8]* @0 to [4 x [4 x i8]]*), i64 0, i64 0, i64 %1
+  %3 = load i8, i8* %2, align 1
+  ret i8 %3
+}