Re: [PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Thomas Haller <thaller@redhat.com>]

On Tue, 2023-08-29 at 21:14 +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 29, 2023 at 09:10:26PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 29, 2023 at 08:54:11PM +0200, Thomas Haller wrote:
> > > An enum can be either signed or unsigned (implementation
> > > defined).
> > > 
> > > datatype_lookup() checks for invalid type arguments. Also check,
> > > whether
> > > the argument is not negative (which, depending on the compiler it
> > > may
> > > never be).
> > > 
> > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > > ---
> > >  src/datatype.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/datatype.c b/src/datatype.c
> > > index ba1192c83595..91735ff8b360 100644
> > > --- a/src/datatype.c
> > > +++ b/src/datatype.c
> > > @@ -87,7 +87,7 @@ const struct datatype *datatype_lookup(enum
> > > datatypes type)
> > >  {
> > >         BUILD_BUG_ON(TYPE_MAX & ~TYPE_MASK);
> > >  
> > > -       if (type > TYPE_MAX)
> > > +       if ((uintmax_t) type > TYPE_MAX)
> > 
> >             uint32_t ?

The more straight forward way would be

    if (type < 0 || type > TYPE_MAX)

However, if the enum is unsigned, then the compiler might see that the
condition is never true and warn against that. It does warn, if "type"
were just an "unsigned int". I cannot actually reproduce a compiler
warning with the enum (for now).

The size of the enum is most likely int/unsigned (or smaller, with "-
fshort-enums" or packed). Is it on POSIX/Linux always guaranteed that
an int is 32bit? I think not, but I cannot find an architecture where
int is larger either. Also, if someone would add an enum value larger
than the 32 bit range, then the behavior is compiler dependent, but
most likely the enum type would be a 64 bit integer and
"uint"/"uint32_t" would not be the right check.

All of this is highly theoretical. But "uintmax_t" avoids all those
problems and makes fewer assumptions on what the enum actually is. Is
there a hypothetical scenario where it wouldn't work correctly?

> 
> Another question: What warning does clang print on this one?
> Description does not specify.

this one isn't about a compiler warning. Sorry, I should not have
included it in this set.


Thomas