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

[Steve Lhomme]

On 2019-07-12 7:59, Thomas Guillem wrote:
> 
> 
> On Fri, Jul 12, 2019, at 07:40, Steve Lhomme wrote:
>> On 2019-07-11 20:50, Rémi Denis-Courmont wrote:
>>> Le torstaina 11. heinäkuuta 2019, 9.51.10 EEST Steve Lhomme a écrit :
>>>> On 2019-07-10 17:36, Rémi Denis-Courmont wrote:
>>>>> 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.
>>>> So you'd be OK with a compiler error when building as long as it's not a
>>>> static_assert ?
>>>
>>> No. I think the code should be adjusted to generate a warning like normal
>>> implicit pointer type conversions do.
>>>
>>> And as already explained, this can easily be achieved with an (static inline)
>>> dummy identity function that expects the correct pointer type. That would also
>>> handle null pointers correctly.
>>>
>>>> And yes, once we agree on something we should generalize
>>>> it to all the set_callbacks() calls.
>>>
>>>> This doesn't compile:
>>>>
>>>> static void func(void*, int) {}
>>>> func(1);
>>>
>>> In that context, func(1) cannot be valid, so it is an error.
>>>
>>>> This compiles (with a warning):
>>>>
>>>> typedef void (*func_ptr)(int);
>>>> static void func(void*, int) {}
>>>> func_ptr call = func;
>>>> call(1);
>>>
>>> And in that context, 'func_ptr call = func' is possibly correct, although that
>>> would be ugly and esoteric.
>>
>> This is exactly how we use the (de)activate callbacks. With no checks at
>> all, hoping for the best. Yes, it's ugly.
>>
>>    Likewise, the 'call(1)' line is fine, in isolation.
>>>
>>> Of course, the combination of both is UB but that's beyond the scope of the
>>> warning.
>>>
>>>> I don't see how one wouldn't want the same behavior for both uses.
>>>>
>>>> And it's easy to see the security hazard when 1 is used a pointer by the
>>>> receiving function.
>>>
>>> I don't see how a code path that would literally always crash if evaluated
>>> would constitute a security hazard. Not all bugs are security issues.
>>
>> This is an example to illustrate how not caring about callback types can
>> lead to crashes even though there was no warning during compilation. Of
>> course this particular example would be easy to pinpoint.
> 
> And sometimes, doing wrong casts don't lead to crashes. Maybe it should be an error, but no it's C so it's a warning. We should stick to it. See -Werror-cast=something if you want to change it (but then, we have to fix all bad casts in VLC, there should not be that many).

Done https://code.videolan.org/robUx4/vlc/tree/callbacks/1
(works with Linux and Win32 compilations)

My point is that right now we have no warning (let alone errors) on 
callbacks and its bad.

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