obs-studio icon indicating copy to clipboard operation
obs-studio copied to clipboard

libobs: Reject plugins compiled with newer libobs

Open norihiro opened this issue 3 years ago • 8 comments

Description

Check obs_module_ver and reject if it returned a larger major/minor version so that plugins build with newer libobs won't be loaded. Patch version is ignored since the API should not be modified by a new patch version.

Note that it might be better to increment LIBOBS_API_VER in the new major beta version (such as 28.0.0-beta, 29.0.0-beta) so that the plugins built with the new major beta version is not loaded by the old OBS.

Motivation and Context

When getting a plugins that is compiled with a newer libobs version in the future, such plugins should not be loaded to avoid potential crash.

This change will block users to accidentally install a new plugins while using old OBS version.

Table below shows examples how

OBS API version Plugin API version Load plugin?
27.2.4 27.2.0 Load
27.2.4 28.0.0 Load (This PR is not implemented.)
28.0.0 27.2.0 Load
28.0.0 28.0.0 Load
28.0.0 28.0.1 Load (The patch version is ignored.)
28.0.0 28.1.0 Rejected (Minor version is compared.)
28.0.0 29.0.0 Rejected
29.0.0 27.2.0 Load
29.0.0 28.0.0 Load
29.0.0 29.0.0 Load

Instead of this PR, each plugin can reject to be loaded by checking OBS's version. This approach depends plugin developers but they have more control.

bool obs_module_load(void)
{
        if (obs_get_version() < (LIBOBS_API_VER & 0xFFFF0000))
                return false;
        // loading something
        return true;
}

How Has This Been Tested?

Overwrite LIBOBS_API_VER on a 3rd paty plugin and compile it, then checked the plugin is not loaded.

#define LIBOBS_API_VER MAKE_SEMANTIC_VERSION(28, 0, 0)
OBS_DECLARE_MODULE()

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • [x] My code has been run through clang-format.
  • [x] I have read the contributing document.
  • [x] My code is not on the master branch.
  • [x] The code has been tested.
  • [x] All commit messages are properly formatted and commits squashed where appropriate.
  • [x] I have included updates to all appropriate documentation.

norihiro avatar Aug 02 '22 19:08 norihiro

There are some potential edge cases here like a plugin being compiled on CI against the latest patch release which was only released for one operating system (for example OBS 27.2.3 being Windows-only). So I'd suggest that this should only look at the major and minor version and ignore patch.

derrod avatar Aug 02 '22 19:08 derrod

I think there's a couple issues with this:

  • Our recent discussions only involve plugins that rely on Qt5 because of the coming Qt6 upgrade, but all plugins are caught in the crossfire now.
  • Plugins that would normally work are now gated by this version check, which means plugin authors are now required to continually bump the version and make new releases that they wouldn't have to before. EDIT: Which also means users would lose access to those plugins if they upgrade OBS until those plugin updates are released.

@Fenrirthviti mentioned something about the planned plugin manager addressing this in some fashion although I don't know the details.

jpark37 avatar Aug 02 '22 21:08 jpark37

Partly because of the above, the draft for the plugin manager would have handled the compatibility checks as part of the plugin manifest for the plugin manager (i.e. the plugin metadata that the plugin manager uses), but that obviously wouldn't work for self-installed plugins or during a migration, so this probably needs some further discussion on the best way to handle. Maybe a combination of both approaches.

Fenrirthviti avatar Aug 02 '22 22:08 Fenrirthviti

  • Plugins that would normally work are now gated by this version check, which means plugin authors are now required to continually bump the version and make new releases that they wouldn't have to before. EDIT: Which also means users would lose access to those plugins if they upgrade OBS until those plugin updates are released.

I believe you got that the wrong way around. This PR blocks plugins compiled against a newer libobs version from loading, not older. So existing plugins are unaffected, but going forward, should there be another big breaking change, a plugin built for a newer obs will no longer load in an older one.

Due to minor changes in patch versions, I believe this check should be restricted to Major/Minor however.

derrod avatar Aug 02 '22 23:08 derrod

Oh, I did have it backwards. Never mind everything I said then. I also think patch should be removed from the check though.

jpark37 avatar Aug 02 '22 23:08 jpark37

So I'd suggest that this should only look at the major and minor version and ignore patch.

Should only patch-version masked or maybe minor version as well?

Plugins that would normally work are now gated by this version check, which means plugin authors are now required to continually bump the version and make new releases that they wouldn't have to before. EDIT: Which also means users would lose access to those plugins if they upgrade OBS until those plugin updates are released.

This is opposite. Old plugins are not gated by this PR. The case the plugin does not work is if plugin developers bump the version, and if user still uses old OBS.

Even without this PR, plugin-load sometimes partially fails because of sizeof(struct obs_source_info), for example, when the plugin developer chooses newer libobs version than users' OBS version. This PR will strongly reject loading such plugins entirely.

This PR does not solve the issue on Qt5 vs. Qt6 unless back-ported to the release/27.2 branch. This PR will avoid future issues similar to the Qt5 vs. Qt6.

If plugin author want to define OBS version more complexly, they can call obs_get_version() or obs_get_version_string() from obs_module_load and return failure, for example.

norihiro avatar Aug 03 '22 01:08 norihiro

I would keep minor.

jpark37 avatar Aug 03 '22 01:08 jpark37

Updated to ignore patch version of the plugin.

norihiro avatar Aug 03 '22 09:08 norihiro

Need to increment LIBOBS_API_VER in the new major beta version (such as 28.0.0-beta, 29.0.0-beta) so that the plugins built with the new major beta version is not loaded by the old OBS.

The versioning in obs.h is necessary to be considered so that I put this PR draft, though the code itself is ready.

norihiro avatar Sep 27 '22 15:09 norihiro

Need to increment LIBOBS_API_VER in the new major beta version (such as 28.0.0-beta, 29.0.0-beta) so that the plugins built with the new major beta version is not loaded by the old OBS.

The versioning in obs.h is necessary to be considered so that I put this PR draft, though the code itself is ready.

Is this still necessary? What needs to be done to get this PR ready to be merged?

PatTheMav avatar Jan 12 '24 21:01 PatTheMav

The current plugin template specifies the version of libobs to compile against. I think this change is ok to be merged to obs-studio.

norihiro avatar Jan 13 '24 01:01 norihiro

The current plugin template specifies the version of libobs to compile against. I think this change is ok to be merged to obs-studio.

What would be the impact for plugins who are not based on the template?

PatTheMav avatar Jan 17 '24 22:01 PatTheMav

What would be the impact for plugins who are not based on the template?

With this PR, if a plugin is build against a newer libobs major-minor version than the running OBS, the plugin will be rejected and a warning message will be left on the log. Without this PR, such a plugin is usually loaded but this would be dangerous as the plugin can expect the new feature. Sometimes obs_register_source will ignore to register if the size of struct obs_source_info is bigger than that on the running OBS. Though, obs_register_source returns nothing so the plugin cannot know the source-type was registered or not.

norihiro avatar Jan 18 '24 01:01 norihiro

With this PR, if a plugin is build against a newer libobs major-minor version than the running OBS, the plugin will be rejected and a warning message will be left on the log.

That does sound right to me.

@RytoEX WDYT?

PatTheMav avatar Jan 27 '24 16:01 PatTheMav