On Wed, Apr 18, 2018 at 12:47:41PM +0100, Frediano Ziglio wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>
> This change addresses two 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,
>
> [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)
>
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> ---
> include/spice-streaming-agent/plugin.hpp | 31 ++++++++++++-------------------
> src/concrete-agent.cpp | 17 ++++++++++++++++-
> src/concrete-agent.hpp | 5 +----
> src/mjpeg-fallback.cpp | 3 ---
> 4 files changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
> index e08e3a6..90651dd 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -102,20 +102,6 @@ public:
> 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.
> */
> @@ -135,19 +121,26 @@ typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);
> }} // namespace spice::streaming_agent
>
> #ifndef SPICE_STREAMING_AGENT_PROGRAM
> +/*!
> + * Plugin interface version
> + * Each plugin should define this variable and set it to PluginVersion
> + * 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;
> +
This probably belongs to another commit.
> #endif
>
> #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 4cf70e7..58ce9c4 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -32,7 +32,7 @@ ConcreteAgent::ConcreteAgent()
>
> bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
> {
> - unsigned version = Version();
> + unsigned version = PluginVersion;
> return MajorVersion(version) == MajorVersion(pluginVersion) &&
> MinorVersion(version) >= MinorVersion(pluginVersion);
> }
> @@ -83,6 +83,21 @@ 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 (!PluginVersionIsCompatible(*version)) {
> + syslog(LOG_ERR,
> + "error loading plugin %s: plugin interface version %u.%u not accepted",
> + plugin_filename.c_str(),
> + MajorVersion(*version), MinorVersion(*version));
> + 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..c0cf9ba 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -27,17 +27,14 @@ 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:
> + bool PluginVersionIsCompatible(unsigned pluginVersion) const;
> void LoadPlugin(const std::string &plugin_filename);
> std::vector<std::shared_ptr<Plugin>> plugins;
> std::vector<ConcreteConfigureOption> options;
> 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;
> -
This bit is not really related to this change, the plugin is static,
agent->Version and PluginVersion will always be the same iirc, so the
check can be dropped because of this.
Apart from this, this is very similar from my split attempt a few weeks
ago.
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel