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

[Rémi Denis-Courmont]

Hi,

I don't want an assertion for an error handler and I don't want an untested error handler when the code can trivially avoid needing one.

Which is exactly the current situation. With size_t, you can't overflow. If it's not broken, don't fix it.

Le 28 septembre 2020 01:51:37 GMT+03:00, Lyndon Brown <jnqnfe at gmail.com> a écrit :
>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).
>
>_______________________________________________
>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é.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200928/ee622d02/attachment.html>