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

[Lyndon Brown]

On Wed, 2020-09-30 at 12:00 +0200, Alexandre Janniaux wrote:
> Hi,
> 
> On Tue, Sep 29, 2020 at 01:33:04AM +0100, Lyndon Brown wrote:
> > On Mon, 2020-09-28 at 09:07 +0200, Alexandre Janniaux wrote:
> > > Hi,
> > > 
> > > In addition to Rémi's argument, I'll add that saving 16 bytes
> > > for a hundred plugins by adding more core to handle errors,
> > > while we are usually allocating more frames than needed is a
> > > micro optimization bringing complexity more than anything,
> > > which is quite different from a «free» save of space.
> > 
> > Hi,
> > 
> > As mentioned, the limit check (whether an assert or otherwise)
> > serves
> > two purposes. 1) catching unexpected overflow with the smaller
> > type, 2)
> > adding a missing check to catch unlikely bugs causing a runaway
> > generation of items (as unlikely as I feel that would be to ever
> > happen), rather than allow it to reach an OOM condition
> 
> I'm not sure I understand the point 2). With size_t, since you cannot
> allocate more than what it can describes, you can never reach a
> condition in which it would overflow, meaning that it would fail
> before
> at the allocation anyway (and there is nothing possible to fix here).
> That's Rémi's point from the beginning.

Sorry, to be more clear, point 1 was about overflow in connection with
the smaller count types, not with the existing size_t type.

With point 2, as a simple artificial example, imagine you place a loop
in a plugin descriptor around an add option entry, with a large loop
count, large enough that it would keep generating items until no more
can be added because there's not enough memory to do so. After chugging
through that work, VLC is then sitting there hogging a vast chunk of
memory, struggling to do anything more, impacting other apps, etc.

This other side (point 2) of the error/assert addition is that it
better bullet proofs the plugin descriptor handling against anything
that would cause such a situation, by simply limiting the number of
items that a plugin can add to a sensible amount, just as we have other
limiters, such as for shortcuts, though too many shortcuts is a more
realistic problem.

As I've previously said, I don't expect we'd ever encounter such a
problem in the real world, and of course there are many bad things a
plugin descriptor could do that we can't easily catch, but this is a
problem that is so easy to catch, that it's worth consideration.

> > , just as some
> > other limits are enforced on plugin descriptors. The latter was an
> > enhancement I thought I should submit for consideration anyway, but
> > had
> > the issue of arbitrarily picking a limit. Since the type change
> > needed
> > a limit of UINT16_MAX, for simplicity I just let that be the limit,
> > and
> > unfortunately chose to bundle that into the type change commit
> > without
> > comment, which has not helped clear discussion.
> > 
> > If we consider the second purpose of the check to be a good idea,
> > which
> > is an entirely separate matter to changing the types, then the
> > change
> > of types is purely a change of types, no addition of checks.
> > 
> > > Regards,
> > > --
> > > Alexandre Janniaux
> > > Videolabs
> > > 
> > > On Sat, Sep 26, 2020 at 10:41:23PM +0100, Lyndon Brown wrote:
> > > > attached. preview:
> > > > 
> > > > From: Lyndon Brown <jnqnfe at gmail.com>
> > > > Date: Wed, 10 Apr 2019 03:48:17 +0100
> > > > Subject: modules: use 16-bit ints for option counts
> > > > 
> > > > nothings is going to need more than UINT16_MAX options, let's
> > > > be
> > > > sensible
> > > > and save the space. note, i may have instead used 8-bit ints,
> > > > but
> > > > that's
> > > > way too small for the core set.
> > > > 
> > > > added an assert on size when adding modules in entry.c to be
> > > > safe
> > > > though.
> > > > 
> > > > on 64-bit this saves 16 bytes per plugin (of which there are
> > > > typically
> > > > hundreds). i.e. we cut the storage space for this plugin conf
> > > > data
> > > > to half
> > > > of what it currently is.
> > > > 
> > > > diff --git a/src/modules/entry.c b/src/modules/entry.c
> > > > index fc7f3cdbb6..cb06085b9c 100644
> > > > --- a/src/modules/entry.c
> > > > +++ b/src/modules/entry.c
> > > > @@ -142,6 +142,8 @@ static module_config_t
> > > > *vlc_config_create(vlc_plugin_t *plugin, int type)
> > > >      unsigned confsize = plugin->conf.size;
> > > >      module_config_t *tab = plugin->conf.items;
> > > > 
> > > > +    assert(confsize < UINT16_MAX);
> > > > +
> > > >      if ((confsize & 0xf) == 0)
> > > >      {
> > > >          tab = realloc_or_free (tab, (confsize + 17) * sizeof
> > > > (*tab));
> > > > diff --git a/src/modules/modules.h b/src/modules/modules.h
> > > > index 93cc1ca289..752b138c84 100644
> > > > --- a/src/modules/modules.h
> > > > +++ b/src/modules/modules.h
> > > > @@ -40,9 +40,9 @@ typedef struct vlc_plugin_t
> > > >      struct
> > > >      {
> > > >          module_config_t *items; /**< Table of configuration
> > > > items
> > > > */
> > > > -        size_t size; /**< Total count of all items */
> > > > -        size_t count; /**< Count of real options (excludes
> > > > hints)
> > > > */
> > > > -        size_t booleans; /**< Count of options that are of
> > > > boolean
> > > > type */
> > > > +        uint16_t size; /**< Total count of all items */
> > > > +        uint16_t count; /**< Count of real options (excludes
> > > > hints) */
> > > > +        uint16_t booleans; /**< Count of options that are of
> > > > boolean type */
> > > >      } conf;
> > > > 
> > > >  #ifdef HAVE_DYNAMIC_PLUGINS
> > > > _______________________________________________
> > > > 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
> > 
> > _______________________________________________
> > 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