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

[Thomas Guillem]

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

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel