[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”.

Well, if we want to play it super-safe, the only way is to mark any change in the Agent interface as binary incompatible. And maybe that’s what we will end up doing, it’s a separate choice.

> 
> 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!).

Why do you find it misleading?

The precise wording is "The current plugin version check violates the C++ ODR”. It specifically talks about “current plugin version check”. And there is indeed no ODR violation in the version check after that change.

Whether there are ODR violations elsewhere, and whether we accept such violations purposefully on a case by case basis for other calls is a judgement call. The mechanism makes it entirely possible to avoid ODR violations completely (at the cost of unnecessarily restricting binary compatibility).

So I don’t see it as misleading or implying that it magically gets rid of other ODR violations.


> 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 ;)

Your understanding is correct. 

Frediano also remarked that it would be nice to add some wording to clarify which changes we thought were safe. At the moment, I have this: https://github.com/c3d/spice-streaming-agent/commit/65ce0c9d747baaa81a4789d7f7c18f6a4f49732c.

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