[Crash-utility] Extensions ABI compatibility

[Mathias Krause]

Hi list,

during development of the PaX module patch[1], Lianbo pointed out an
issue about adding new members to struct offset_table. They should be
added to the end to ensure binary compatibility with already compiled
extensions, as these won't be aware of the new members and, even worse,
might be using wrong offsets within the offset_table if we didn't. As
this seems to be rather fragile, here's a proposal to make it less so.

To not blindly make already compiled extensions misbehave by using the
wrong offsets when new members get added in the middle of shared types,
we need to have some validation step, preferably during extension
registration to catch these.

If we want to keep binary compatibility, we need to add members to the
end. That's a rule maintainers can enforce but as we all make mistakes,
code can help us here. Changing a shared data structure has, at least,
implications on its size. So we can use this as a base to build checks
on. If the size of a data structure used during compilation of an
extension differs from the size of that same data structure as seen
within 'crash', we have an ABI issue. The response should be, at least,
a warning to inform the user to recompile its extension to ensure ABI
compatibility. Another option would be to decline loading the extension
but that might be inconvenient for users, if we indeed made sure to add
members only at the end of the involved data structures.

These checks can be implemented "transparently" as sketched in the
attached patch. It changes register_extension() to be a macro that
stores the sizes of critical types in a data structure that gets passed
to _register_extension() which is implemented in 'crash' and will do the
size checks against its version of the involved types.

The patch also takes care about backward compatibility by providing a
stub register_extension() function that passes a NULL structure, as all
current extension modules refer to register_extension() instead of the
newly introduced _register_extension().

Open TODOs:
1/ Compile a list of all types we want to protect and add checks for
   these.
2/ Put these types into a 'shared.h' header file and use that
   exclusively within extensions -- maybe even enforce it.

Do we want to follow that approach or look for alternatives?

Regards,
Mathias

[1]: https://www.redhat.com/archives/crash-utility/2020-August/msg00024.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: abi_size_check.diff
Type: text/x-patch
Size: 2914 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20200819/623227f4/attachment.bin>