immunant / IA2-Phase2

5 stars 0 forks source link

(WIP) rewriter: add `__attribute__((visibility("default"))` to `IA2_FN` address taken functions if they don't already default visibility #445

Closed kkysen closed 1 month ago

kkysen commented 1 month ago

I tried copying the similar approach used to insert the __attribute__((used))s, and while its finding the right functions to add the __attribute__((visibility("default")))s to, the replacements are going in totally wrong places places, and I'm not sure what I'm doing wrong or quite how the replacements API is supposed to work. @ayrtonm or anyone else, do you have any suggestions?

ayrtonm commented 1 month ago

Do we actually need fvisibility on the address taken functions? I think it's only the wrappers that need it.

ayrtonm commented 1 month ago

Oh it's needed for non-static address taken functions. It might be better to define the call gates in the same .c like we do for static functions that way we don't have to prepend attributes to function definitions.

I'm guessing the issues you're running into probably stem from expansion/spelling loc differences. Doing these kinds of rewrites gets trickier when functions are defined in headers/expanded from macros so I'd like to avoid them if possible.

ayrtonm commented 1 month ago

It might be better to define the call gates in the same .c like we do for static functions

I just realized that these may be referenced from multiple .c files so we need to either:

  1. arbitrarily pick a .c per compartment for non-static address-taken function call gates to put the call gate wrappers in
  2. or generate a .c per compartment and link that in

The first option is kinda ugly but I'm leaning towards that for expediency. @rinon thoughts?

ayrtonm commented 1 month ago

For context I mostly want to avoid prepending attributes to functions with the rewriter to avoid cases like #414 where a function's definition has static or some attribute expanded from a macro and we can't reliably tell if the macro expansion includes anything else.

ayrtonm commented 1 month ago

Superseded by #451.