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

Romain Vimont rom1v at videolabs.io
Thu Jul 11 11:35:36 CEST 2019 

On Thu, Jul 11, 2019 at 08:51:10AM +0200, Steve Lhomme wrote:
> 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 ? 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);
> 
> 
> This compiles (with a warning):
> 
> typedef void (*func_ptr)(int);
> static void func(void*, int) {}
> func_ptr call = func;
> call(1);
> 
> 
> I don't see how one wouldn't want the same behavior for both uses.

I agree it would be better if it were an error, but then I think the
solution would be to request the compiler to consider casting between
incompatible pointer types as an error, not to add macros/_Generic for
one specific case:

    -Werror=incompatible-pointer-types

Would this cause any problem?

> And it's easy to see the security hazard when 1 is used a pointer by the
> receiving function.
> 
> > 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é.
> > 
> > 
> > _______________________________________________
> > 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

More information about the vlc-devel mailing list