[Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check
Christophe de Dinechin
cdupontd at redhat.com
Wed Apr 18 09:56:49 UTC 2018
> On 17 Apr 2018, at 20:11, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>>
>>> On 17 Apr 2018, at 16:12, Frediano Ziglio <fziglio at redhat.com> 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
>>>>
>>>
>>> This is clear to everybody, doing from the agent avoid each plugin to
>>> do the check on the version and we can do it safely before executing
>>> any code in the plugin (library initialization aside).
>>
>> As I pointed out in an early reply, library initialization is not much of an
>> issue since it does not have access to the agent yet. So barring static
>> entry points in the agent interface that could be called during init, we are
>> OK.
>>
>>>
>>>> 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.
>>>>
>>>> [1]
>>>> https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
>>>>
>>>
>>> Reading all the replies looks like to me that what you are basically
>>> doing with these changes is basically removing the minor from the
>>> versioning
>>> schema and making every change binary incompatible.
>>
>> I don’t understand how you could reach that conclusion. If that was the case,
>> there would be no “PluginInterfaceOldestCompatibleVersion” version in my
>> proposal.
>>
>> In reality, the numbering change I proposed offers the possibility to decide,
>> for any release, what is the oldest compatible version, without having to
>> necessarily have that oldest compatible be the latest.
>>
>> In the example I gave above, I was indicating that version 20 was compatible
>> with version 3, but that version 21 introduced a change that breaks
>> compatibility not with version 20, but with versions earlier than 13. This
>> can be expressed with my scheme easily by bumping the “oldest compatible”
>> from 3 to 13 (without being forced to bump it to 21). By contrast, with the
>> major.minor scheme, this kind of scenario requires special treatment.
>>
>> When does this kind of scenario happen? Imagine we added support for
>> loading/unloading plugins. Version 13 adds a new interface to unload
>> plugins, and a new “unloadable plugin” entry point. Therefore, starting with
>> version 13, the agent would try the new entry point first, and if it’s
>> there, assume it can unload the plugin, otherwise fallback to the old method
>> and mark the plugin as not-unloadable. Therefore, the change in version 13
>> would still be binary compatible with version 3. Years later, in version 21,
>> we decide to drop support for non-unloadable plugins. So now the agent
>> refuses to load a plugin if it does not have the new entry point. In that
>> case, we’d want to bump the “compatible version” to 13, since all plugins
>> after 13 have that new entry point. The key win here is that we’d not have
>> to force the last compatible version to be 21.
>>
>
> I think this is more a MinimalVersion definition than a compatibility
> version.
“Minimal version” is quite confusing in that context. Minimal implies some sort of “reduced functionality”, which is not what we are talking about here. “Oldest compatible version” is much clearer IMO.
> And by the way can be implemented also with major.minor, just the minimal
> would be 1.13 instead of 13.
As I explained earlier, you could indeed special-case version 1.13 in the plugin version check. This ends up implementing an “oldest compatible” version using major.minor. If what we really care is “what is the oldest compatible version”, then that’s what we should have in the code, and having “major.minor” instead is just obfuscating the real intent.
>> This could mean years of differences in terms of binary compatibility. We
>> could deprecate things slowly and offer graceful transitions.
>>
>>
>>>
>>> libtool by itself uses major, minor to implement its numbering schema
>>> that is the major minor schema is more flexible than libtool schema.
>>> Major minor allows for instance to have different plugins or executables
>>> installed
>>> each in its major private directory or having multiple binary library
>>> installed in a system.
>>>
>>> If a plugin is compiled with version X and the agent if version Y
>>> (obviously
>>> having X != Y) a full ODR would require the definition of "Agent" (for
>>> instance)
>>> in version X would be the same of version Y. That is each version is not
>>> compatible.
>>
>> That has already been discussed with Christophe F. A full ODR is not the
>> objective. The objective is to avoid violating the ODR while checking if the
>> ODR violations we have are acceptable. Or as I wrote earlier, the code
>> testing for binary compatibility should not assume some kind of binary
>> compatibility to start with.
>>
>
> Okay, this patch solves ODR for PluginVersionIsCompatible. But I think or we
> consider ODR as a issue or not.
As I replied to Christophe, there isn’t a single answer to the general ODR question.
But there is a single answer to the question “is it an issue to rely on the plugin being compatible with the agent while checking if the plugin is compatible with the agent”. I hope that it’s obvious *that* is an issue :-)
The patch also prevents plugins from bypassing version check. Let's not forget that second bug. Given the lack of an open source model at present, I think it matters a lot.
> The practical question is, having:
>
> class Something {
> public:
> virtual void do_something(int param)=0;
> };
>
> how to extend it avoiding ODR violation and ABI change?
> That is how do you plan to extend the interface in a compatible way?
Defining our binary compatibility strategy is completely orthogonal to the patch. So I would like for the patch to move forward without being dragged into a completely different topic.
That being said, FWIW, the original strategy of adding methods at the end of the class works for GCC, so that’s a good starting point. But there can be more subtle cases, e.g. thread safety, versions of libc or libcxx, etc. So the general answer will take judgement, and we will learn over time.
> Simply if you want to break the initial ABI to check the version you could
> change the entry point name. Either way I agree that reading the version
> with an entry point is safer.
I don’t understand what you mean or suggest here. The key to the fix is checking the version before using the entry point, i.e. the opposite of "reading the version with an entry point” (I’m not reading the version with the plugin entry point, but without it). Or maybe you meant “reading the version with a dl symbol”?
>
> Exporting an entry point from a library with a given name and structure
> (in this case an unsigned number) is an ABI so the "the code
> testing for binary compatibility should not assume some kind of binary
> compatibility to start with" is basically doing the opposite assuming
> an ABI.
I am not “assuming” an ABI, I am *checking* it. In other words, I am verifying if the symbol exists, with defined behaviour if it does not, and then checking the value for that symbol, with defined behaviour if the value is out of range.
So I’m really not assuming much, and specifically, much less than the present solution. Notably, unlike the present code, the proposed solution does not
1. make assumptions on the Agent class vtable,
2. require the plugin init to call some specific method,
3. require the plugin init to check the return value of said method,
4. require the plugin init to faithfully report whether it thinks it’s compatible or not.
5. require the above steps to be done correctly without any valid open-source example to use as a model
>
>>>
>>>> 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
>>>> +};
>>>>
>>>> enum Ranks : unsigned {
>>>> /// this plugin should not be used
>>>> @@ -103,20 +114,6 @@ class Agent
>>>> {
>>>> public:
>>>> /*!
>>>> - * Get agent version.
>>>> - * Plugin should check the version for compatibility before doing
>>>> - * everything.
>>>> - * \return version specified like PluginVersion
>>>> - */
>>>> - virtual unsigned Version() const = 0;
>>>> -
>>>> - /*!
>>>> - * Check if a given plugin version is compatible with this agent
>>>> - * \return true is compatible
>>>> - */
>>>> - virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const
>>>> =
>>>> 0;
>>>> -
>>>> - /*!
>>>> * Register a plugin in the system.
>>>> */
>>>> virtual void Register(Plugin& plugin) = 0;
>>>> @@ -136,18 +133,25 @@ typedef bool
>>>> PluginInitFunc(spice::streaming_agent::Agent* agent);
>>>>
>>>> #ifndef SPICE_STREAMING_AGENT_PROGRAM
>>>> /*!
>>>> + * Plugin interface version
>>>> + * Each plugin should define this variable and set it to
>>>> PluginInterfaceVersion
>>>> + * That version will be checked by the agent before executing any plugin
>>>> code
>>>> + */
>>>> +extern "C" unsigned spice_streaming_agent_plugin_interface_version;
>>>> +
>>>> +/*!
>>>> * Plugin main entry point.
>>>> - * Plugins should check if the version of the agent is compatible.
>>>> - * If is compatible should register itself to the agent and return
>>>> - * true.
>>>> - * If is not compatible can decide to stay in memory or not returning
>>>> - * true (do not unload) or false (safe to unload). This is necessary
>>>> + * This entry point is only called if the version check passed.
>>>> + * It should return true if it loaded and initialized successfully.
>>>> + * If the plugin does not initialize and does not want to be unloaded,
>>>> + * it may still return true on failure. This is necessary
>>>> * if the plugin uses some library which are not safe to be unloaded.
>>>> * This public interface is also designed to avoid exporting data from
>>>> * the plugin which could be a problem in some systems.
>>>> * \return true if plugin should stay loaded, false otherwise
>>>> */
>>>> extern "C" spice::streaming_agent::PluginInitFunc
>>>> spice_streaming_agent_plugin_init;
>>>> +
>>>> #endif
>>>>
>>>> #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
>>>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
>>>> index 4cf70e7..eb4f333 100644
>>>> --- a/src/concrete-agent.cpp
>>>> +++ b/src/concrete-agent.cpp
>>>> @@ -15,28 +15,11 @@
>>>>
>>>> using namespace spice::streaming_agent;
>>>>
>>>> -static inline unsigned MajorVersion(unsigned version)
>>>> -{
>>>> - return version >> 8;
>>>> -}
>>>> -
>>>> -static inline unsigned MinorVersion(unsigned version)
>>>> -{
>>>> - return version & 0xffu;
>>>> -}
>>>> -
>>>> ConcreteAgent::ConcreteAgent()
>>>> {
>>>> options.push_back(ConcreteConfigureOption(nullptr, nullptr));
>>>> }
>>>>
>>>> -bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion)
>>>> const
>>>> -{
>>>> - unsigned version = Version();
>>>> - return MajorVersion(version) == MajorVersion(pluginVersion) &&
>>>> - MinorVersion(version) >= MinorVersion(pluginVersion);
>>>> -}
>>>> -
>>>> void ConcreteAgent::Register(Plugin& plugin)
>>>> {
>>>> plugins.push_back(std::shared_ptr<Plugin>(&plugin));
>>>> @@ -83,6 +66,24 @@ void ConcreteAgent::LoadPlugin(const std::string
>>>> &plugin_filename)
>>>> return;
>>>> }
>>>>
>>>> + unsigned *version =
>>>> + (unsigned *) 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) {
>>>> + syslog(LOG_ERR,
>>>> + "error loading plugin %s: plugin interface version %u, "
>>>> + "agent accepts version %u...%u",
>>>> + plugin_filename.c_str(), *version,
>>>> + PluginInterfaceOldestCompatibleVersion,
>>>> + PluginInterfaceVersion);
>>>> + return;
>>>> + }
>>>> +
>>>> try {
>>>> PluginInitFunc* init_func =
>>>> (PluginInitFunc *) dlsym(dl,
>>>> "spice_streaming_agent_plugin_init");
>>>> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
>>>> index 5bca23b..c631916 100644
>>>> --- a/src/concrete-agent.hpp
>>>> +++ b/src/concrete-agent.hpp
>>>> @@ -27,16 +27,12 @@ class ConcreteAgent final : public Agent
>>>> {
>>>> public:
>>>> ConcreteAgent();
>>>> - unsigned Version() const override {
>>>> - return PluginVersion;
>>>> - }
>>>> void Register(Plugin& plugin) override;
>>>> const ConfigureOption* Options() const override;
>>>> void LoadPlugins(const std::string &directory);
>>>> // pointer must remain valid
>>>> void AddOption(const char *name, const char *value);
>>>> FrameCapture *GetBestFrameCapture(const std::set<SpiceVideoCodecType>&
>>>> codecs);
>>>> - bool PluginVersionIsCompatible(unsigned pluginVersion) const
>>>> override;
>>>> private:
>>>> void LoadPlugin(const std::string &plugin_filename);
>>>> std::vector<std::shared_ptr<Plugin>> plugins;
>>>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>>>> index 68c282f..605a4b3 100644
>>>> --- a/src/mjpeg-fallback.cpp
>>>> +++ b/src/mjpeg-fallback.cpp
>>>> @@ -180,9 +180,6 @@ SpiceVideoCodecType MjpegPlugin::VideoCodecType()
>>>> const {
>>>>
>>>> bool MjpegPlugin::Register(Agent* agent)
>>>> {
>>>> - if (agent->Version() != PluginVersion)
>>>> - return false;
>>>> -
>>>> std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
>>>>
>>>> try {
>>>
>>> Maybe a way to avoid ODR violation would be to define interface versions in
>>> a
>>> chained way like
>>
>> This is addressing a different issue, which is how to extend the interface in
>> C++ without violating the ODR. Your approach does not really solve the ODR
>> problem, it moves it to the interface of the plugin entry point. Version 1
>> plugins would have a function that takes an Agent_V1 pointer, version 2
>> plugins would expect an Agent_V2. You constructed them to be binary
>> compatible, but that’s not different from adding the fields directly to the
>> class in a way that makes it binary compatible.
>>
>> That does not make your approach invalid. It gets bonus points for making it
>> clear what the version changes are.
>>
>
> Not only that, Agent_V1 is always defined the same way, avoiding ODR violation.
> Even if compiler decides to order the functions by an hash of the name for instance
> the definition is the same so the compiler has to use the same ABI.
Indeed, but a plugin compiled against V1 would then assume the V1 vtable layout, yet get a V2 object. So that would still be an ODR violation.
You could make it work by having differently-named plugin entry points, i.e. plugin_init_V1(Agent_V1 *), plugin_init_V2(Agent_V2 *), and so on. At which point you completely lose any benefit you might have expected.
Or you could decide that plugin_init always takes an Agent_V1, in which case the compiler compiling the agent would know that it has to pass a valid Agent_V1 class pointer and do any required this-pointer adjustment. That would be somewhat inconvenient as a V2-capable plugin would then have to then cast the pointer back to Agent_V2. So it would be quite inconvenient and confusing, and use explicit casts, negating the expected benefits of inheritance. But then there would be no ODR violation…
>
>>>
>>> class Agent_V1 {
>>> public:
>>> func1
>>> func2
>>> };
>>>
>>> class Agent_V2: public Agent_V1 {
>>> public:
>>> func3
>>> func4
>>> };
>>>
>>> typedef Agent_V2 Agent;
>>>
>>> enum Versions {
>>> Current = Version_2;
>>> };
>>>
>>> The only not ODR would be the typedef
>>
>> The real concern with ODR would be the plugin entry point that uses that
>> typedef.
>>
>
> That's why I suggested to always pass Agent_V1.
That’s not what the code example says, it says “typedef Agent_V2 Agent”. Since your code snippet did not provide an updated interface for the plugin init function, I assumed it was still taking an Agent pointer, therefore an Agent_V2 with the typedef above. Now, if you meant for the plugin init to always take an Agent_V1, then yes, you fix the violation that way, and the plugins have to up-cast to whatever version they want.
However, if the agent upcast to its desired version, then your suggestion for Agent_V3 : public Agent_V1 breaks: a V2 plugin would try to cast the V1 pointer to a V2 pointer and get garbage… Which I guess is why you said you’d refuse any plugin using version 2 (I initially thought you refused to load V2 because it was broken).
> Or the agent should pass the proper pointer based on the version read from the plugin.
It does not scale well. I can see this getting complicated when we reach version 20 ;-)
>
>>> but does not looks a big issue, the
>>> init function could receive a "Agent_V1" pointer always and each plugin
>>> knows that could convert to its "Agent" type safely as the agent already
>>> checked the ABIs.
>>>
>>> If for some reasons version 2 is buggy the agent can have a:
>>>
>>> class Agent_V3: public Agent_V1 {
>>> ...
>>> };
>>>
>>> and refuse to load any plugin using version 2.
It was not an objective for me to accept compatibility ranges with holes in them (i.e., in your example, V3 compatible with V1 but not V2). I think it would be a terrible idea and a maintenance nightmare.
>>>
>>> The agent could use some adapter interface to support multiple plugin
>>> interfaces.
>>
>> Yes, this is what I used as an example for “unloadable plugins” above.
>>
>>
>> Christophe
>>
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list