[vlc-devel] [PATCH 2/6] modules: check plugin version
Steve Lhomme
robux4 at ycbcr.xyz
Fri Jul 19 12:57:04 CEST 2019
On 2019-07-19 12:19, Marvin Scholz (ePirat) wrote:
>
>
>> Am 19.07.2019 um 12:08 schrieb Steve Lhomme <robux4 at ycbcr.xyz>:
>>
>>> On 2019-07-19 11:33, Rémi Denis-Courmont wrote:
>>> And vlc_dlsym does a string match. Exact same logic for the version check.
>>
>> I don't deny that. But there is an extra string comparison in your code.
>>
>>> In practice, this fails miserably. Either the plugin will not load because of missing symbols (most common), or we'll crash or corrupt the process before we can dlsym(). No change there.
>>
>> Having a generic entry point has its benefits. But that also mean we have stronger backward/forward compatibility checks. For example I sometimes have old plugins lying around in my build folder. If the version doesn't match it won't be a problem in the current code. It will realize the new entry point is not there and won't go further. In the new system it will actually load it and call the version callback.
>
> Actually the current behavior is imo worse, it causes to popup error dialogs on Windows which is very confusing for the user.
On what version of Windows ? Because I think on all Windows version
supported by 4.0 we set the error mode so that it doesn't happen.
In any case changing from "vlc_entry__4_0_4" to "vlc_entry__4_0_5" or
"vlc_entry_api_version" will not change anything in the code when trying
to load older DLLs.
> Even though since the installer was fixed this is not a very common problem anymore, I am anyway in favor of the new approach as it seems to me in general like a better way to handle this.
>
>>
>>> Le 19 juillet 2019 12:25:02 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>>>> On 2019-07-19 11:15, Rémi Denis-Courmont wrote:
>>>>> Hi,
>>>>>
>>>>> It was a string with a string comparison before (inside vlc_dlsym).
>>>> It still is a string comparison.
>>>>
>>>> In the current code we do:
>>>> - vlc_dlopen
>>>> - vlc_dlsym to load the callback that has the version in its name
>>>>
>>>> In the new code we do:
>>>> - vlc_dlopen
>>>> - vlc_dlsym to load the callback with a generic name
>>>> - strcmp to check the version of the module
>>>>
>>>>> Of course it could be changed to an integer, but then the meta export
>>>> macro won't be usable, so it will need more custom code.
>>>>>
>>>>> Le 19 juillet 2019 10:04:48 GMT+03:00, Steve Lhomme
>>>> <robux4 at ycbcr.xyz> a écrit :
>>>>>> On 2019-07-18 21:29, Rémi Denis-Courmont wrote:
>>>>>>> ---
>>>>>>> src/modules/bank.c | 59
>>>>>> +++++++++++++++++++++++++++++++++++-----------
>>>>>>> 1 file changed, 45 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/modules/bank.c b/src/modules/bank.c
>>>>>>> index 0b551a91bc..f6f5ae1a85 100644
>>>>>>> --- a/src/modules/bank.c
>>>>>>> +++ b/src/modules/bank.c
>>>>>>> @@ -193,6 +193,49 @@ static void module_InitStaticModules(void) { }
>>>>>>> #endif
>>>>>>> #ifdef HAVE_DYNAMIC_PLUGINS
>>>>>>> +static const char *module_GetVersion(void *handle)
>>>>>>> +{
>>>>>>> + const char *(*get_api_version)(void);
>>>>>>> +
>>>>>>> + get_api_version = vlc_dlsym(handle, "vlc_entry_api_version");
>>>>>>> + if (get_api_version == NULL)
>>>>>>> + return NULL;
>>>>>>> +
>>>>>>> + return get_api_version();
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void *module_Open(struct vlc_logger *log,
>>>>>>> + const char *path, bool fast)
>>>>>>> +{
>>>>>>> + void *handle = vlc_dlopen(path, fast);
>>>>>>> + if (handle == NULL)
>>>>>>> + {
>>>>>>> + char *errmsg = vlc_dlerror();
>>>>>>> +
>>>>>>> + vlc_error(log, "cannot load plug-in %s: %s", path,
>>>>>>> + errmsg ? errmsg : "unknown error");
>>>>>>> + free(errmsg);
>>>>>>> + return NULL;
>>>>>>> + }
>>>>>>> +
>>>>>>> + const char *str = module_GetVersion(handle);
>>>>>>> + if (str == NULL) {
>>>>>>> + vlc_error(log, "cannot load plug-in %s: %s", path,
>>>>>>> + "unknown version or not a plug-in");
>>>>>>> +error:
>>>>>>> + vlc_dlclose(handle);
>>>>>>> + return NULL;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (strcmp(str, VLC_API_VERSION_STRING)) {
>>>>>>
>>>>>> Couldn't we use an integer ? Speed-wise this is going backward from
>>>>>> have
>>>>>> no extra check needed because the name we loaded already had the
>>>> proper
>>>>>>
>>>>>> version, which was a feature not a bug.
>>>>>>
>>>>>>> + vlc_error(log, "cannot load plug-in %s: unsupported
>>>> version
>>>>>> %s", path,
>>>>>>> + str);
>>>>>>> + goto error;
>>>>>>> + }
>>>>>>> +
>>>>>>> + return handle;
>>>>>>> +}
>>>>>>> +
>>>>>>> static const char vlc_entry_name[] = "vlc_entry" MODULE_SUFFIX;
>>>>>>> /**
>>>>>>> @@ -207,15 +250,9 @@ static const char vlc_entry_name[] =
>>>> "vlc_entry"
>>>>>> MODULE_SUFFIX;
>>>>>>> static vlc_plugin_t *module_InitDynamic(vlc_object_t *obj, const
>>>>>> char *path,
>>>>>>> bool fast)
>>>>>>> {
>>>>>>> - void *handle = vlc_dlopen(path, fast);
>>>>>>> + void *handle = module_Open(obj->logger, path, fast);
>>>>>>> if (handle == NULL)
>>>>>>> - {
>>>>>>> - char *errmsg = vlc_dlerror();
>>>>>>> - msg_Err(obj, "cannot load plug-in %s: %s", path,
>>>>>>> - errmsg ? errmsg : "unknown error");
>>>>>>> - free(errmsg);
>>>>>>> return NULL;
>>>>>>> - }
>>>>>>> /* Try to resolve the symbol */
>>>>>>> vlc_plugin_cb entry = vlc_dlsym(handle, vlc_entry_name);
>>>>>>> @@ -507,15 +544,9 @@ int module_Map(struct vlc_logger *log,
>>>>>> vlc_plugin_t *plugin)
>>>>>>> /* Try to load the plug-in (without locks, so read-only) */
>>>>>>> assert(plugin->abspath != NULL);
>>>>>>> - void *handle = vlc_dlopen(plugin->abspath, false);
>>>>>>> + void *handle = module_Open(log, plugin->abspath, false);
>>>>>>> if (handle == NULL)
>>>>>>> - {
>>>>>>> - char *errmsg = vlc_dlerror();
>>>>>>> - vlc_error(log, "cannot load plug-in %s: %s",
>>>>>>> - plugin->abspath, errmsg ? errmsg : "unknown
>>>>>> error");
>>>>>>> - free(errmsg);
>>>>>>> return -1;
>>>>>>> - }
>>>>>>> vlc_plugin_cb entry = vlc_dlsym(handle, vlc_entry_name);
>>>>>>> if (entry == NULL)
>>>>>>> --
>>>>>>> 2.22.0
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> vlc-devel mailing list
>>>>>>> To unsubscribe or modify your subscription options:
>>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>>>
>>>>>> _______________________________________________
>>>>>> vlc-devel mailing list
>>>>>> To unsubscribe or modify your subscription options:
>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>
>>>>> --
>>>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>>>> excuser ma brièveté.
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> vlc-devel mailing list
>>>>> To unsubscribe or modify your subscription options:
>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>> --
>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list