Page MenuHomePhabricator

[dfsan] Correctly copy attributes to variadic custom wrapper
AbandonedPublic

Authored by blukat29 on Feb 1 2018, 4:45 AM.

Details

Reviewers
pcc
dneilson
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

Diff Detail

Event Timeline

blukat29 created this revision.Feb 1 2018, 4:45 AM
blukat29 edited the summary of this revision. (Show Details)Feb 1 2018, 5:07 AM
blukat29 updated this revision to Diff 134826.Feb 18 2018, 12:41 AM
blukat29 retitled this revision from [dfsan] Correctly copy attributes when calling variadic custom wrapper to [dfsan] Correctly copy attributes to variadic custom wrapper.
blukat29 edited the summary of this revision. (Show Details)
blukat29 added a reviewer: dneilson.

Rebased to revision 325456.

dneilson accepted this revision.Feb 22 2018, 10:52 AM

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!

This revision is now accepted and ready to land.Feb 22 2018, 10:52 AM
pcc added a comment.Feb 22 2018, 11:09 AM

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 requested changes to this revision.Feb 22 2018, 12:18 PM
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!

This revision now requires changes to proceed.Feb 22 2018, 12:18 PM
blukat29 abandoned this revision.Feb 22 2018, 6:25 PM

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