DataFlowSanitizer custom trampoline functions incompatible with opaque pointers

[nikic]

DataFlowSanitizer supports an (apparently undocumented?) feature where function-pointer-typed arguments to functions with custom ABI will be converted into two arguments, one being a trampoline function, and the other the original argument cast to a void pointer. I believe the intention is that the user will then call the trampoline function, while passing the original function, the call arguments, plus any necessary labels, and the trampoline will take care of the necessary __dfsan_arg_tls / __dfsan_retval_tls writes.

Unfortunately, this is incompatible with opaque pointers, because the form of the trampoline is determined based on the function pointer type. I think this is also a bit problematic at a higher level, because pointer types are also arbitrary in C, not just in IR (i.e. you could pass in a callback as void* and later cast it, or more problematically, pass it in with one signature and then cast it before the call).

I'm not entirely sure what the right way to resolve this is. My first inclination would be that we should remove this automatic rewrite in instrumentation, and instead provide a helper for invoking callbacks in custom functions in dfsan.h. So you'd do something like dfsan_call(cb, ...args, ...labels, ret_label) in the custom function. This might come in the form of a builtin (I'm not sure if it's possible to implement dfsan_call as just C++?)

However, I'm not familiar with dfsan, so I'm not sure whether the above is possible, both on a technical level, and in terms of stability -- is that dfsan interface considered stable?

The alternative I see here would be to do something very sad, like require the frontend to annotate everything with elementtype attributes or some metadata when dfsan is enabled.

cc @aeubanks
cc @pcc who added this functionality in https://reviews.llvm.org/D1503
cc @DerSaidin who has recent DFSan activity

[DerSaidin]

For convenience, opaque pointers references: 1 2

My first inclination would be that we should remove this automatic rewrite in instrumentation, and instead provide a helper for invoking callbacks in custom functions in dfsan.h. So you'd do something like dfsan_call(cb, ...args, ...labels, ret_label) in the custom function. This might come in the form of a builtin (I'm not sure if it's possible to implement dfsan_call as just C++?)

I think this would work, and is probably a good option. I would welcome @pcc's thoughts here.

I think this could be done in C++, with template parameter packs, but it may get messy to ensure the function args stay in sync with the label args.. Edit: Actually, aggregate types may need a builtin.

In terms of stability, we probably haven't been very explicit about stability of DFSan custom functions. In practice, I imagine nearly all usage of this functionality is within DFSan to wrap libc functions. I think this change will be possible.

I'll take a look at switching from trampoline functions to using a helper to invoke callbacks in custom functions.

[DerSaidin]

DFSan no longer uses pointer types to generate trampoline functions.

In the future we may want to add a built in. For now I just packed the shadow/origin values into TLS manually, because there is only one case that needs this (Other callbacks in dfsan_custom.cpp want to zero out shadow/origin values in TLS).

[nikic]

Thanks @DerSaidin!