> On 23 Mar 2018, at 16:49, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > >> >> From: Christophe de Dinechin <dinechin@xxxxxxxxxx> >> >> 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 just proves that the ABI changed, not that the check is robust, I'd remove this comment. It proves that the ABI change is *detected*, a sanity test, if you wish. (I talked about “indication”, not “proof”). > Is more robust as the init function is not called before checking the version but you already said this above. It’s also more robust because it verifies that the version variable is there. It’s a distinct test. > OT: Technically to call dlsym you already loaded the shared object into > memory so you could in theory already execute some code form old plugin > but if at this stage you are not able to unload it is a bug in the plugin. Yes, but the code you load has no way to access the agent. > >> 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) >> > > > How in the new schema are you updating the interface? In any way you want. You just need to know if the change is binary compatible or not, which is largely a judgment call on the part of the committer. It’s documented in the comment, although re-reading it, I believe that I can reword it to give more guidance: /*! * 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 change is made to this header, increment PluginInterfaceVersion::Current. * * [BINARY INCOMPATIBLE CHANGE] If the changes made to this header are not binary compatible, * set PluginInterfaceVersion::OldestCompatible to PluginInterfaceVersion::Current * * [DETECTED INCOMPATIBILITY]: If an incompatibility is detected after a release, * set PluginInterfaceVersion::OldestCompatible to the last known compatible version. * * [BINARY COMPATIBILITY GUIDELINES] * The following changes can be considered binary compatible: * - Adding new file-scope or namespace-scope entities, e.g. variables, functions, classes * - Adding new methods at the end of the Agent class (GCC-compatible compilers) * - Adding new values at the end of an existing enum * - Adding typedefs when that does not change any existing type in the file */ Any things worth mentioning that you can think of worth adding to that list? > Is not clear how this patch is going to solve this, can you provide an example? The purpose of the patch is not to solve the problem of binary compatibility, but to be able to reliably detect incompatibility. > >> 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. >> > > typos: incompatiliy, compatibiliy Tehse are copmilcatde worsd! > >> 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 >> >> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx> >> --- >> 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. > > It seems that changing PluginInterfaceOldestCompatibleVersion is like > bumping the major but I think I'll get the reply when I got an explanation > how the interface is updated. As explained, the difference is this (with typos fixed): >> If later testing shows that 1.13 actually introduced an incompatibly, you have to special-case 1.13 in the compatibility check. In other words, you don’t have to bump it to the current. If at version 21, you detect that you are no longer compatible all the way back to 7, but only al the way back to 13, you can enforce that, instead of having to bump all the way to 21, or special case “1.13” in your version check. Imagine you used a deprecated symbol in the plugin interface up until version 12, and fixed it in version 13. In version 21, the agent itself stops using the deprecated symbol, at which point you can run on systems where the symbol is no longer available, but only if plugins are version 13 or above. (This is a gross oversimplification of a real-life scenario involving to a re-implementation of libcxx). > >> */ >> -enum Constants : unsigned { PluginVersion = 0x100u }; >> +enum Constants : unsigned { >> + PluginInterfaceVersion = 1, >> + PluginInterfaceOldestCompatibleVersion = 1 > > Does the plugin needs to know PluginInterfaceOldestCompatibleVersion? No, but I want the two values to be in the same enum (enum class), and I see no good reason to hide it. If we move it elsewhere where plugins can’t see it, there is a risk of forgetting to update it. > >> +}; >> >> 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 { > > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel