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

[Christophe de Dinechin]

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

Imagine you check for a null pointer. If you write:

	if (ptr)
		return ptr->foo();
	else
		return 0;

that’s OK, because you test the pointer before using it. There is no undefined behavior in C++. However, if instead you write:

	int Foo::foo()
	{
		if (this)
			…
		else
			return 0;
	}

then there is a violation of C++ semantics. It may work with some compilers, some other compilers will optimize away the ‘if (this)’ test (I believe clang does).

Our current version check looks like the second example, i.e. the test is done too late, so that at the point where we test, we violate C++ semantics and the test does not yield the expected results. In the most severe case, the test in today’s code would crash instead of detecting the version check mismatch.


> Yet it's singled out as an ODR violation that has to be fixed in the commit log.

It’s singled out because the binary-compatibility check should not itself rely on the code being binary compatible :-) So it’s different in that way.

For later ODR violations, it’s OK if the code “trusts” our binary compatibility analysis, whatever that analysis may be. There is no known automated way to detect an ODR violation in the general case, otherwise, compilers would do it…


> By reading it, yes I kind of expect that we are no longer having similar violations anywhere in the code, otherwise we would fix them too.

As I stated in my previous reply, the mechanism makes it *possible* for us to entirely avoid later ODRs. But thats’ a *policy* that you can enforce with the mechanism. It’s a choice. The trade-off, as I also explained in my previous reply, is that if you stick to the strictest ODR policy, then practically any change in the Agent class will be marked as binary incompatible. That policy would be fine with me, btw, but with g++, it’s probably a bit too paranoid, and not what Frediano for example initially intended.

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


> Imo what really matters here is that we need this check to detect ABI changes. which are problematic from an ODR point of view, it's not the ODR violation by itself. But it really is just a matter of wording.

The existing initial ODR violation is problematic inasmuch as its primary objective is to assess if potential ODR violations are acceptable (“binary compatible”) or not (“binary incompatible”).

Anyway, a better wording is welcome, as long as it is not less precise or correct than the current one. Since I do not fully understand what you find misleading in it, I cannot offer a suggestion.

Also, let’s make sure we are not once more spiralling into fruitless bikeshedding. If the patch is correct and fixes an actual bug, and if the wording is not blatantly incorrect (i.e. after fixing the spelling mistakes Frediano pointed out), we might want to get the fix in instead of arguing for days about it.


Christophe “had successfully avoided wearing a C++ lawyer hat for nearly 18 years, and was not too unhappy about it"

> 
> Christophe