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

[Christophe Fergeau]

On Thu, Mar 29, 2018 at 10:36:06AM +0200, Christophe de Dinechin wrote:
> 
> 
> > On 29 Mar 2018, at 10:05, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Wed, Mar 28, 2018 at 05:54:21PM +0200, Christophe de Dinechin wrote:
> >>> 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.
> > 
> > As long as we don't change the ABI, the version check is as much
> > an ODR violation as any other part of the code.
> 
> In the present code, we violate the ODR *before* being able to assess if that’s an ODR violation we are OK with.

Yes, this is also what I was saying ;) Though we could alternatively
mandate that the first few fields of the class must never change, in
which case the version check would still be an ODR violation, but one we
are fine with.

> In any case, could you suggest a wording that you think would be less “misleading”?

I would use something like this:

 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
    any Agent method from the plugin 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. As long as the
    agent/plugin ABI does not change, this is deemed acceptable and
    should not cause actual runtime issues.

    However, the current version check (which is done through
    Agent::PluginVersionIsCompatible) relies on the layout of the Agent
    class not changing, which puts unnecessary constraints on the changes
    allowed in the classes when breaking ABI (e.g. it's not allowed to
    put anything before some member functions). Given that this version
    check is used to detect ABI incompatibilities, it's better to make
    it rely as little as possible on the ABI.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180329/c43a37fc/attachment.sig>