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

[Alexandre Janniaux]

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.

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