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

[Rémi Denis-Courmont]

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

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/