[vlc-devel] [PATCH 2/6] modules: check plugin version

Marvin Scholz epirat07 at gmail.com
Fri Jul 19 17:07:11 CEST 2019



On 19 Jul 2019, at 13:43, Rémi Denis-Courmont wrote:

> 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.

Oh ok, then I misunderstood the changes, sorry.

>
> 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é._______________________________________________
> 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