[vlc-devel] [PATCH v4 3/5] display: add macro to set the callbacks and check type of the Open/Close

[Rémi Denis-Courmont]

Hi,

I don't see how wrong pointer cast is a security hazard. If the target fails to validate function prototypes at runtime, having proper function prototypes at compile time is not going to help. Conversely, if the target enforces type matching, then it will crash without exploit routes.

But the main point is that this is not specific to vout display open callback. If you think that function type mismatch should be an error, then use the corresponding -Werror-whatever option accordingly. We already have one such case in the build system for missing/implicit declarations.

Le 10 juillet 2019 15:07:59 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>On 2019-07-10 13:51, Rémi Denis-Courmont wrote:
>> Hi,
>> 
>> A static_assert failure *is* an error. 
>
>"more than an error" is an error.
>
>Mismatched function pointer types should be a warning, not an error.
>> 
>
>I strongly disagree. This is a security hazard left unattended.
>
>> You're free to use Werror on your builds if you want.
>> 
>> Le 10 juillet 2019 14:35:17 GMT+03:00, Steve Lhomme
><robux4 at ycbcr.xyz> a écrit :
>>> On 2019-07-10 12:42, Rémi Denis-Courmont wrote:
>>>> Hi,
>>>>
>>>> As already stated yesterday, I don't see why this specific case
>>> grants an error any more than any other function pointer passing.
>>>
>>> It's more than an error, it's a static assert. It won't compile if
>you
>>> use the wrong signature. IMO it's a good step in better code safety.
>>>
>>> One drawback of this approach is when you change a signature you
>have
>>> to
>>> change it in all modules, whereas before you could change the
>signature
>>>
>>> and then each module individually. Not breaking compilation, just
>the
>>> code running temporarily.
>>>
>>> Warnings are usually ignored (see my other patches fixing long time
>>> warnings).
>>>
>>> We could have a static_assert in debug build and a warning in
>release
>>> builds.
>>>
>>>> In any case, this wrongly causes an error if zero is passed as
>>> deactivate callback or if NULL is an integer.
>>>
>>> And if NULL is passed instead of nullptr in C++. It's easy enough to
>>> use
>>> the proper value in that case.
>>>
>>> For NULL I wasn't sure if it's always a void* in C11. It is in Clang
>>> (linux, win32), gcc (win32) and MSVC.
>>>
>>> There's typeof(NULL) but it seems to be a GNU extension :
>>>
>https://stackoverflow.com/questions/40143815/is-it-possible-to-implement-gnu-cs-typeofx-with-c11s-generic
>>>
>>>> Le 10 juillet 2019 11:39:06 GMT+03:00, Steve Lhomme
>>> <robux4 at ycbcr.xyz> a écrit :
>>>>> ---
>>>>> include/vlc_vout_display.h | 17 +++++++++++++++++
>>>>> 1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/include/vlc_vout_display.h
>b/include/vlc_vout_display.h
>>>>> index fd4940cf6d..1cbd6b1891 100644
>>>>> --- a/include/vlc_vout_display.h
>>>>> +++ b/include/vlc_vout_display.h
>>>>> @@ -215,6 +215,23 @@ typedef int
>>> (*vout_display_open_cb)(vout_display_t
>>>>> *vd,
>>>>>    */
>>>>> typedef void (*vout_display_close_cb)(vout_display_t *vd);
>>>>>
>>>>> +#ifndef __cplusplus
>>>>> +#define set_callbacks_display(Activate, Deactivate) \
>>>>> +    static_assert( _Generic( Activate, vout_display_open_cb: 1,
>>>>> default: 0 ) == 1, \
>>>>> +                  "Not a display activate callback"); \
>>>>> +    static_assert( _Generic( Deactivate, vout_display_close_cb:
>1,
>>>>> void*: 1, default: 0 ) == 1, \
>>>>> +                  "Not a display deactivate callback"); \
>>>>> +    set_callbacks( Activate, Deactivate )
>>>>> +#else /* __cplusplus */
>>>>> +#define set_callbacks_display(Activate, Deactivate) \
>>>>> +    static_assert(
>>>>> std::is_same<std::add_pointer<decltype(Activate)>::type,
>>>>> vout_display_open_cb>::value, \
>>>>> +                  "Not a display activate callback"); \
>>>>> +    static_assert(
>>>>> std::is_same<std::add_pointer<decltype(Deactivate)>::type,
>>>>> vout_display_close_cb>::value || \
>>>>> +
>>>>> std::is_same<decltype(Deactivate),std::nullptr_t>::value, \
>>>>> +                  "Not a display deactivate callback"); \
>>>>> +    set_callbacks( Activate, Deactivate )
>>>>> +#endif /* __cplusplus */
>>>>> +
>>>>> struct vout_display_t {
>>>>>       struct vlc_object_t obj;
>>>>>
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> 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

-- 
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/20190710/507bae82/attachment.html>