| Message ID | 20230829185509.374614-6-thaller@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | fix compiler warnings with clang and "-Wextra" | expand |
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 ? > return NULL; > return datatypes[type]; > } > -- > 2.41.0 >
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 ? Another question: What warning does clang print on this one? Description does not specify. > > return NULL; > > return datatypes[type]; > > } > > -- > > 2.41.0 > >
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
On Tue, Aug 29, 2023 at 09:58:53PM +0200, Thomas Haller wrote: > 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). Then, better keep it back? > 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. I don't expect to ever have such a large number of types. Specifically because there are API restrictions that apply in this case. > 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? I was trying to figure out what this is fixing. > > 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. This TYPE_MAX will not ever become very large to require 64-bits. With an implementation where enum is taken as signed, then this should be sufficient too: if (type > TYPE_MAX) If this is not fixing up anything right now, I would prefer to keep this back. I'll take this series except this one. Thanks.
On Wed, 2023-08-30 at 09:46 +0200, Pablo Neira Ayuso wrote: > On Tue, Aug 29, 2023 at 09:58:53PM +0200, Thomas Haller wrote: > > 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). > > Then, better keep it back? > > > 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. > > I don't expect to ever have such a large number of types. > Specifically > because there are API restrictions that apply in this case. > > > 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? > > I was trying to figure out what this is fixing. > > > > 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. > > This TYPE_MAX will not ever become very large to require 64-bits. TYPE_MAX is not relevant. > With an implementation where enum is taken as signed, then this > should > be sufficient too: > > if (type > TYPE_MAX) > > If this is not fixing up anything right now, I would prefer to keep > this back. I don't think it suffices. The following fail the assertion (or would access out of bounds). diff --git c/include/datatype.h i/include/datatype.h index 9ce7359cd340..7d3b6b20d27c 100644 --- c/include/datatype.h +++ i/include/datatype.h @@ -98,7 +98,8 @@ enum datatypes { TYPE_TIME_HOUR, TYPE_TIME_DAY, TYPE_CGROUPV2, - __TYPE_MAX + __TYPE_MAX, + __TYPE_FORCE_SIGNED = -1, }; #define TYPE_MAX (__TYPE_MAX - 1) diff --git c/src/datatype.c i/src/datatype.c index ba1192c83595..1ff8a4a08551 100644 --- c/src/datatype.c +++ i/src/datatype.c @@ -89,6 +89,7 @@ const struct datatype *datatype_lookup(enum datatypes type) if (type > TYPE_MAX) return NULL; + assert(type != (enum datatypes) -1); return datatypes[type]; } diff --git c/src/libnftables.c i/src/libnftables.c index 9c802ec95f27..7e60d1a18d39 100644 --- c/src/libnftables.c +++ i/src/libnftables.c @@ -203,6 +203,8 @@ struct nft_ctx *nft_ctx_new(uint32_t flags) #endif } + datatype_lookup(-1); + ctx = xzalloc(sizeof(struct nft_ctx)); nft_init(ctx); If you expect that "type" is always valid, then there is no need to check against >TYPE_MAX. If you expect that it might be invalid, it seems prudent to also check against negative values. Thomas
On Wed, Aug 30, 2023 at 10:08:50AM +0200, Thomas Haller wrote: [...] > I don't think it suffices. The following fail the assertion (or would > access out of bounds). > > > diff --git c/include/datatype.h i/include/datatype.h > index 9ce7359cd340..7d3b6b20d27c 100644 > --- c/include/datatype.h > +++ i/include/datatype.h > @@ -98,7 +98,8 @@ enum datatypes { > TYPE_TIME_HOUR, > TYPE_TIME_DAY, > TYPE_CGROUPV2, > - __TYPE_MAX > + __TYPE_MAX, > + __TYPE_FORCE_SIGNED = -1, I don't expect to ever have a negative defined here. > }; > #define TYPE_MAX (__TYPE_MAX - 1) > > diff --git c/src/datatype.c i/src/datatype.c > index ba1192c83595..1ff8a4a08551 100644 > --- c/src/datatype.c > +++ i/src/datatype.c > @@ -89,6 +89,7 @@ const struct datatype *datatype_lookup(enum datatypes > type) > > if (type > TYPE_MAX) > return NULL; > + assert(type != (enum datatypes) -1); > return datatypes[type]; > } > > diff --git c/src/libnftables.c i/src/libnftables.c > index 9c802ec95f27..7e60d1a18d39 100644 > --- c/src/libnftables.c > +++ i/src/libnftables.c > @@ -203,6 +203,8 @@ struct nft_ctx *nft_ctx_new(uint32_t flags) > #endif > } > > + datatype_lookup(-1); > + > ctx = xzalloc(sizeof(struct nft_ctx)); > nft_init(ctx); > > > > > If you expect that "type" is always valid, then there is no need to > check against >TYPE_MAX. If you expect that it might be invalid, it > seems prudent to also check against negative values. > > > > Thomas >
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) return NULL; return datatypes[type]; }
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(-)