[Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check

[Christophe de Dinechin]

> On 28 Mar 2018, at 17:20, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Wed, Mar 28, 2018 at 11:10:36AM +0200, Christophe de Dinechin wrote:
>> 
>> 
>>> On 26 Mar 2018, at 19:06, Christophe Fergeau <cfergeau at redhat.com> wrote:
>>> 
>>> On Fri, Mar 23, 2018 at 01:05:23PM +0100, Christophe de Dinechin wrote:
>>>> 2. ODR-related problems
>>>> 
>>>> The C++ One Definition Rule (ODR) states that all translation units
>>>> must see the same definitions. In the current code, when we call
>>>> Agent::PluginVersionIsCompatible from the plugin, it is an ODR
>>>> violation as soon as we have made any change in the Agent class
>>>> compared to what the plugin was compiled against.
>>>> 
>>>> The current code therefore relies on implementation dependent knowlege
>>>> of how virtual functions are laid out in the vtable, and puts
>>>> unnecessary constraints on the changes allowed in the classes
>>>> (e.g. it's not allowed to put anything before some member functions)
>>> 
>>> Just to be sure I understand everything properly, as I could not figure
>>> it out fully from the commit log... The problem at the moment is that we
>>> expect the plugins to call Agent::PluginVersionIsCompatible(), but if
>>> the agent ABI drastically changed, we won't be able to do that as the
>>> layout of the Agent when the plugin was built and the new layout of the
>>> Agent won't match. Given that Agent::PluginVersionIsCompatible() is
>>> supposed to be used to detect incompatible ABIs, this could be an issue.
>> 
>> What you are describing is a practical case where we would crash or
>> call the wrong function or whatever.
>> 
>> However, in reality, calling PluginVersionIsCompatible is “undefined
>> behavior” as soon as the interface of the class changed in any way
>> relative to what the plugin was compiled with. That’s what “One
>> Definition Rule” means.
>> 
>> Now, to be clear, I’m not advocating that we should mark any change in
>> the plugin interface as binary incompatible. Adding a method at the
>> end of the agent interface, for example, should be safe with g++, so
>> we could say that the ABI version with the added method is “binary
>> compatible” with the previous one.
> 
> Yup, sounds reasonable.
> 
>> What I’m advocating is that we should not rely on undefined behavior to perform that binary compatibility test.
> 
> Yes, I agree, I was just making sure I understood your point properly.
> 
>>> However, my understanding is that once the version check has succeeded, we
>>> assume that ODR violations are not going to cause issues, because we
>>> have validated through that version check that the ABI is compatible.
>>> In other word, without any version check, we could have issues with
>>> calling Agent::Register() from a plugin, but thanks to the version
>>> check, we assume that this ODR violation is not going to cause actual
>>> issues?
>> 
>> We do not assume. We test it first, and based on that test, we decide
>> whether we mark the version binary compatible or not.
>> 
>> Relying on the ODR means that the language does not specify what
>> happens. We can, however, make a judgment call based for example on
>> the generated code and on our testing, and *after* that judgement call
>> decide to call the agent methods.
>> 
>> Does that make sense?
> 
> I feel more comfortable with a statement like "as long as we don't break
> ABI, we are going to assume that the plugin and agent are going to work
> fine together, and that we won't be getting ODR problems". Testing can
> only be done with a limited set of compiler and flags, so I won't feel
> so confident saying "it worked fine on my machine, it's going to work
> fine everywhere".
> 
> With that said, I find the current "ODR" portion of the commit log
> misleading, it's easy to get the impression that after this change, we
> won't have any ODR problem anymore (this is what I initially thought!).
> The paragraph
> "The current code therefore relies on implementation dependent knowlege
>    of how virtual functions are laid out in the vtable, and puts
>    unnecessary constraints on the changes allowed in the classes
>    (e.g. it's not allowed to put anything before some member
>    functions)"
> is probably already plenty enough to describe why we want it, I would
> just be explicit that "the changes allowed in the classes" are "the
> changes allowed in the classes *when breaking ABI*" (or at least I
> understand it this way ;)

That’s correct.

> 
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel