[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

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

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

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

Okay, sounds plausible.

[fhahn]

lib/Analysis/ConstantFolding.cpp

551

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

Shouldn't this be

opt < %s -sccp -S

[asbirlea]

Address comment.

test/Transforms/SCCP/ubsan_overflow.ll

1

The behavior is the same.

[asbirlea]

Added needed cast from unsigned to int64_t.

[sanjoy.google]

lib/Analysis/ConstantFolding.cpp

551

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

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

13

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!

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 >= InitializerSize)
     return UndefValue::get(IntType);
 
   // If we're not accessing anything in this constant, the result is undefined.
-  if (Offset >= InitializerSize)
+  if (Offset + BytesLoaded <= 0)
     return UndefValue::get(IntType);
 
   unsigned char RawBytes[32] = {0};
   unsigned char *CurPtr = RawBytes;
   unsigned BytesLeft = BytesLoaded;
 
   // If we're loading off the beginning of the global, some bytes may be valid.
   if (Offset < 0) {
     CurPtr += -Offset;

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 void @foo() local_unnamed_addr {
+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
+  unreachable
+}