[vlc-devel] [PATCH] modules: use 16-bit ints for option counts

Lyndon Brown jnqnfe at gmail.com
Mon Sep 28 00:51:37 CEST 2020


On Sun, 2020-09-27 at 23:38 +0300, Rémi Denis-Courmont wrote:
> Le sunnuntaina 27. syyskuuta 2020, 23.03.24 EEST Lyndon Brown a écrit
> :
> > I don't think it's very fair to call the patch broken, an assert is
> > added to the function where options are added into the system as a
> > precaution to catching the unlikely scenario of there ever being
> > such a
> > bug causing a huge volume of creation requests.
> 
> It is broken. It's trivial to construct a scenario where the option
> list will 
> overflow. And as a general rule, we do not use assertions to handle
> errors.

I assume you mean like using a loop in the plugin descriptor? Sure, it
is of course trivially easy to do, but you're only going to end up with
a vast quantity of options if you're deliberately doing stupid things
to mess about, or if you make a mistake doing something very unusual.
Pragmatically it's not something we should expect to ever encounter in
the real world. Regardless, out of caution, this introduced a condition
to catch such a situation, which is arguably something worth adding
anyway.

I can't understand how you can consider that concern to be cause for
calling the patch 'broken', especially when the patch both handles the
situation (even if you don't like how), and arguably does so better.

If you'd prefer handling the error condition to be done in a different
way to asserting, then I would be perfectly happy to make such an
adjustment, you've only got to ask. I'd also be perfectly happy to
split out introduction of the check into it's own commit, since it's a
perfectly good enhancement in its own right. I think the main reason I
went with an assertion was simply because multiple existing parts
of vlc_plugin_desc_cb() make use of assertions to handle unexpected
problems, like too many shortcuts for instance. I actually do already
have another patch to submit later which replaces this assert with
error handling, and I can bring that bit forward if that appeases you.
(In fact as it happens, I've got later work to submit which
revises vlc_plugin_desc_cb() to avoid using assertions entirely such
that a problematic plugin gets rejected rather than bringing the whole
app down).



More information about the vlc-devel mailing list