[dfsan] Correctly copy attributes to variadic custom wrapper

[blukat29]

Summary

When DataFlowSanitizer converts call to a function which has its custom wrapper,
new call instruction directly inherits the attributes from the original call
instruction. However, this attribute copy is incorrect in case of variadic
function because shadow arguments are inserted in the middle of original
arguments.

This wrong copy behavior causes subtle compiler crashes when certain kinds of
attributes (e.g. nonnull) are attached to original varargs and the attributes are
not compatible with shadow arguments.

This bug has been mentioned in following references:
https://bugs.llvm.org/show_bug.cgi?id=28907
http://lists.llvm.org/pipermail/llvm-dev/2017-September/117324.html

[blukat29]

Rebased to revision 325456.

[dneilson]

Not terribly familiar with this code, but my understanding from reading the construction of the CustomCI for WK_Custom...

The new CI that's being constructed has as arguments: All of the regular non-varargs args, then the shadow args for those regular args, and then the varargs from the original CI. The blind setAttribute on line 1534 was a problem because it would end up setting the attributes associated with varargs in the original CI to shadow args in the new CI -- because the attribute list is an ordered list and there was no bubble/hole created in the list to accommodate the shadow args that are inserted between the regular and varargs arguments. So, the solution is to be more careful -- your change is just creating a new attribute list from the old one with the exception that you inject a bubble/hole of empty attribute sets where the shadow arguments would go.

Logic seems good to me. Added code and test both look fine to me.

Thanks for the patch!

[pcc]

Apologies for missing this patch. It seems to be fixing the same problem as D43132, but that patch also handles trampoline functions correctly, so I think we should go with that one.

[dneilson]

In D42789#1016252, @pcc wrote:

Apologies for missing this patch. It seems to be fixing the same problem as D43132, but that patch also handles trampoline functions correctly, so I think we should go with that one.

Agreed. That sounds much better!

[blukat29]

Oh, I see. Then I will close this patch. Thank you for the responses.

lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

case DataFlowSanitizer::WK_Custom:
           Args.push_back(DFSF.LabelReturnAlloca);
         }
 
         for (i = CS.arg_begin() + FT->getNumParams(); i != CS.arg_end(); ++i)
           Args.push_back(*i);
 
         CallInst *CustomCI = IRB.CreateCall(CustomF, Args);
         CustomCI->setCallingConv(CI->getCallingConv());
+
+        if (FT->isVarArg()) {
+          AttributeList OldAttrs = CI->getAttributes();
+          SmallVector<AttributeSet, 8> ArgAttrs;
+          const unsigned NonVarArgCount = FT->getNumParams();
+          const unsigned ShadowArgCount =
+              CustomFT->getNumParams() - NonVarArgCount;
+
+          // Copy non-vararg attributes.
+          for (unsigned n = 0; n < NonVarArgCount; n++) {
+            ArgAttrs.push_back(OldAttrs.getParamAttributes(n));
+          }
+          // Skip label parameters.
+          for (unsigned n = 0; n < ShadowArgCount; n++) {
+            ArgAttrs.push_back(AttributeSet());
+          }
+          // Copy vararg attributes.
+          for (unsigned n = NonVarArgCount; n < CS.arg_size(); n++) {
+            ArgAttrs.push_back(OldAttrs.getParamAttributes(n));
+          }
+
+          CustomCI->setAttributes(AttributeList::get(
+              F->getContext(), OldAttrs.getFnAttributes(),
+              OldAttrs.getRetAttributes(), ArgAttrs));
+        } else {
           CustomCI->setAttributes(CI->getAttributes());
+        }
 
         // Update the parameter attributes of the custom call instruction to
         // zero extend the shadow parameters. This is required for targets
         // which consider ShadowTy an illegal type.
         for (unsigned n = 0; n < FT->getNumParams(); n++) {
           const unsigned ArgNo = ShadowArgStart + n;
           if (CustomCI->getArgOperand(ArgNo)->getType() == DFSF.DFS.ShadowTy)
             CustomCI->addParamAttr(ArgNo, Attribute::ZExt);

test/Instrumentation/DataFlowSanitizer/Inputs/sprintf-abilist.txt

+fun:sprintf=custom
+fun:sprintf=uninstrumented

test/Instrumentation/DataFlowSanitizer/custom-attrs.ll

+; RUN: opt < %s -dfsan -dfsan-args-abi -dfsan-abilist=%S/Inputs/sprintf-abilist.txt -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @f() {
+  ; CHECK: call signext i32 {{.*}} @__dfsw_sprintf(i8* align 1 undef, i8* align 2 undef, i16 zeroext 0, i16 zeroext 0, i16* %{{.*}}, i16* %{{.*}}) #[[ATTR:[0-9]+]]
+  ; CHECK: call signext i32 {{.*}} @__dfsw_sprintf(i8* align 1 undef, i8* align 2 undef, i16 zeroext 0, i16 zeroext 0, i16* %{{.*}}, i16* %{{.*}}, i8* align 4 undef, i8* align 8 undef) #[[ATTR:[0-9]+]]
+  ; attributes #[[ATTR]] = { nounwind }
+  call signext i32 (i8*, i8*, ...) @sprintf(i8* align 1 undef, i8* align 2 undef) #1
+  call signext i32 (i8*, i8*, ...) @sprintf(i8* align 1 undef, i8* align 2 undef, i8* align 4 undef, i8* align 8 undef) #1
+  ret void
+}
+
+declare i32 @sprintf(i8*, i8*, ...)
+attributes #1 = { nounwind }