[Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check
Christophe de Dinechin
christophe.de.dinechin at gmail.com
Mon Mar 26 14:57:30 UTC 2018
> On 23 Mar 2018, at 14:53, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> On Fri, 2018-03-23 at 13:05 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com>
>>
>> This change addresses three issues related to plugin version checking:
>>
>> 1. It is possible for plugins to bypass version checking or do it wrong
>> (as a matter of fact, the mjpeg fallback sets a bad example)
>>
>> 2. The current plugin version check violates the C++ ODR, i.e.
>> it relies on undefined behaviors when the header used to compile
>> the plugin and to compile the agent are not identical,
>>
>> 3. A major.minor numbering scheme is not ideal for ABI checks.
>> In particular, it makes it difficult to react to an incompatibility
>> that was detected post release.
>>
>> [More info]
>>
>> 1. Make it impossible to bypass version checking
>>
>> The current code depends on the plugin implementing the version check
>> correctly by calling PluginVersionIsCompatible. To make things worse,
>> the only publicly available example gets this wrong and uses an
>> ad-hoc version check, so anybody copy-pasting this code will get it
>> wrong.
>>
>> It is more robust to do the version check in the agent before calling
>> any method in the plugin. It ensures that version checking cannot be
>> bypassed, is done consistently and generates consistent error messages.
>>
>> As an indication that the aproach is robust, the new check correctly
>> refuses to load older plugins that use the old version checking method:
>>
>> spice-streaming-agent[5167]:
>> error loading plugin [...].so: no version information
>>
>> 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)
>>
>> 3. Major.minor numbering scheme
>>
>> The major.minor numbering scheme initially selected makes it harder
>> to fixes cases where an incompatibility was detected after release.
>>
>> For example, the major.minor version checking assumes that agent 1.21
>> is compatible with plugins 1.21, 1.13 or 1.03. If later testing
>> shows that 1.13 actually introduced an incompatiliy, you have to
>> special-case 1.13 in the compatibiliy check.
>>
>> An approach that does not have this problem is to rely on incremented
>> version numbers, with a "current" and "oldest compatible" version
>> number. This is used for example by libtools [1].
>>
>> Since the change required for #1 and #2 introduces an ABI break,
>> it is as good a time as any to also change the numbering scheme,
>> since changing it later would introduce another unnecessary ABI break.
>
> Great! AFAIK we haven't made a release yet so we don't need to worry
> about ABI breakage yet?
Well, I’d recommend we take the habit of incrementing if we make significant changes, as a way to warn fellow SPICE developers that something changed.
>
>> [1] https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
>>
>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
>> ---
>> include/spice-streaming-agent/plugin.hpp | 50 +++++++++++++++++---------------
>> src/concrete-agent.cpp | 35 +++++++++++-----------
>> src/concrete-agent.hpp | 4 ---
>> src/mjpeg-fallback.cpp | 3 --
>> 4 files changed, 45 insertions(+), 47 deletions(-)
>>
>> diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
>> index e08e3a6..0ec5040 100644
>> --- a/include/spice-streaming-agent/plugin.hpp
>> +++ b/include/spice-streaming-agent/plugin.hpp
>> @@ -23,11 +23,22 @@ namespace streaming_agent {
>> class FrameCapture;
>>
>> /*!
>> - * Plugin version, only using few bits, schema is 0xMMmm
>> - * where MM is major and mm is the minor, can be easily expanded
>> - * using more bits in the future
>> + * Plugins use a versioning system similar to that implemented by libtool
>> + * http://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
>> + * Update the version information as follows:
>> + * [ANY CHANGE] If any interfaces have been added, removed, or changed since the last update,
>> + * increment PluginInterfaceVersion.
>> + * [COMPATIBLE CHANGE] If interfaces have only been added since the last public release,
>> + * leave PluginInterfaceOldestCompatibleVersion identical.
>> + * [INCOMPATIBLE CHANGE] If any interfaces have been removed or changed since the last release,
>> + * set PluginInterfaceOldestCompatibleVersion to PluginInterfaceVersion.
>> + * [DETECTED INCOMPATIBILITY]: If an incompatibility is detected after a release,
>> + * set PluginInterfaceOldestCompatibleVersion to the last known compatible version.
>> */
>> -enum Constants : unsigned { PluginVersion = 0x100u };
>> +enum Constants : unsigned {
>> + PluginInterfaceVersion = 1,
>> + PluginInterfaceOldestCompatibleVersion = 1
>> +};
>
> This is still not too pretty, consider at least renaming Constants to
> something better, or even use something like:
>
> struct PluginInterfaceVersion {
> static constexpr uint16_t current = 1;
> static constexpr uint16_t oldest_compatible = 1;
> };
>
> For the encapsulation?
The two values are of the same type, and can be compared only with one another, which is what the enum type says. This is completely lost with your suggestion.
That being said, Constants is not a very good name choice, it makes it sound like it’s just a bag of random unrelated constants. And it would be beneficial to use an enum class here to enforce the “compare with one another".
What about the additional patch below?
Author: Christophe de Dinechin <dinechin at redhat.com>
Date: Mon Mar 26 16:26:57 2018 +0200
Use an enumeration class for the plugin interface version
diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
index b01cd82..a98ee58 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -35,9 +35,9 @@ class FrameCapture;
* [DETECTED INCOMPATIBILITY]: If an incompatibility is detected after a release,
* set PluginInterfaceOldestCompatibleVersion to the last known compatible version.
*/
-enum Constants : unsigned {
- PluginInterfaceVersion = 1,
- PluginInterfaceOldestCompatibleVersion = 1
+enum class PluginInterfaceVersion : unsigned {
+ Current = 1,
+ OldestCompatible = 1
};
enum Ranks : unsigned {
@@ -154,8 +154,9 @@ extern "C" spice::streaming_agent::PluginInitFunc spice_streaming_agent_plugin_i
#define SPICE_STREAMING_AGENT_PLUGIN(agent) \
__attribute__ ((visibility ("default"))) \
- unsigned spice_streaming_agent_plugin_interface_version = \
- spice::streaming_agent::PluginInterfaceVersion; \
+ spice::streaming_agent::PluginInterfaceVersion \
+ spice_streaming_agent_plugin_interface_version = \
+ spice::streaming_agent::PluginInterfaceVersion::Current; \
\
__attribute__ ((visibility ("default"))) \
bool spice_streaming_agent_plugin_init(spice::streaming_agent::Agent* agent)
diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index eb4f333..d7b9fbc 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -66,21 +66,21 @@ void ConcreteAgent::LoadPlugin(const std::string &plugin_filename)
return;
}
- unsigned *version =
- (unsigned *) dlsym(dl, "spice_streaming_agent_plugin_interface_version");
+ PluginInterfaceVersion *version =
+ (PluginInterfaceVersion *) dlsym(dl, "spice_streaming_agent_plugin_interface_version");
if (!version) {
syslog(LOG_ERR, "error loading plugin %s: no version information",
plugin_filename.c_str());
return;
}
- if (*version < PluginInterfaceOldestCompatibleVersion ||
- *version > PluginInterfaceVersion) {
+ if (*version < PluginInterfaceVersion::OldestCompatible ||
+ *version > PluginInterfaceVersion::Current) {
syslog(LOG_ERR,
"error loading plugin %s: plugin interface version %u, "
"agent accepts version %u...%u",
plugin_filename.c_str(), *version,
- PluginInterfaceOldestCompatibleVersion,
- PluginInterfaceVersion);
+ PluginInterfaceVersion::OldestCompatible,
+ PluginInterfaceVersion::Current);
return;
}
More information about the Spice-devel
mailing list