[vlc-devel] [PATCH 2/6] modules: check plugin version
Rémi Denis-Courmont
remi at remlab.net
Fri Jul 19 13:43:33 CEST 2019
Hi,
This patch changes absolutely nothing to how runtime linking failures are handled. Such failure occurs before the version check, both with or without this patch.
Le 19 juillet 2019 13:19:26 GMT+03:00, "Marvin Scholz (ePirat)" <epirat07 at gmail.com> a écrit :
>
>
>> 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.
>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
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20190719/e70128b9/attachment.html>
More information about the vlc-devel
mailing list