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

[Lyndon Brown]

On Mon, 2020-09-28 at 09:29 +0300, Rémi Denis-Courmont wrote:
> Hi,
> 
> I don't want an assertion for an error handler ...

As I said, that's fine, very happy to do it differently.

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

Who says I'd not tested it?

You can't overflow, but you can eat up memory until maxed out and OOM
checks are kicking in, which would be undesirable to let happen.

Without meaning for you to take my saying this the wrong way, you were
just arguing how trivially possible it is for a huge quantity of items
to be generated by a plugin. If that's how you feel then surely it must
follow that you'd agree that having a check to catch an excessive large
quantity by capping at a sensible limit would be a good thing, rather
than reaching OOM checks?

Such a check is an entirely separate issue to whether the count types
are changed and possibility of overflow. I wish I'd made this more
clear by separating the capping protection out into it's own commit,
but it was not clear what limit to pick... not separating it, instead
just letting it be a side effect of needing a cap at UINT16_MAX at most
for the type change, avoided having to pick one, since that max I felt
was good enough.

> If it's not broken, don't fix it.

This isn't a fix, it's a pair of enhancements.


> 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
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel