Re: [ovs-dev] [PATCH v12 3/8] shash, simap, smap: Add assertions to `*_count` functions
On 7/13/23 14:57, Eelco Chaudron wrote: > > > On 12 Jul 2023, at 0:34, Ilya Maximets wrote: > >> On 7/11/23 12:05, Eelco Chaudron wrote: >>> >>> >>> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: >>> This commit adds assertions in the functions `shash_count`, `simap_count`, and `smap_count` to ensure that the corresponding input struct pointer is not NULL. This ensures that if the return values of `shash_sort`, `simap_sort`, or `smap_sort` are NULL, then the following for loops would not attempt to access the pointer, which might result in segmentation faults or undefined behavior. Signed-off-by: James Raphael Tiovalen Reviewed-by: Simon Horman --- lib/shash.c | 2 ++ lib/simap.c | 2 ++ lib/smap.c | 1 + 3 files changed, 5 insertions(+) diff --git a/lib/shash.c b/lib/shash.c index a7b2c6458..2bfc8eb50 100644 --- a/lib/shash.c +++ b/lib/shash.c @@ -17,6 +17,7 @@ #include #include "openvswitch/shash.h" #include "hash.h" +#include "util.h" static struct shash_node *shash_find__(const struct shash *, const char *name, size_t name_len, @@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash) size_t shash_count(const struct shash *shash) { +ovs_assert(shash); >>> >>> My preference would be to return 0, in these instances. What do others >>> think? >> >> Calling these function with a NULL argument doesn't make much sense to me. >> free()-like functions should generally accept NULL pointers, but functions >> that actually do work on a datastructure shouldn't, IMO. > > Fine by me too, so with this: > > Acked-by: Eelco Chaudron > Applied this one as well. Thanks, everyone! Will mark remaining 4 patches from this set as 'changes-requested'. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 3/8] shash, simap, smap: Add assertions to `*_count` functions
On 12 Jul 2023, at 0:34, Ilya Maximets wrote: > On 7/11/23 12:05, Eelco Chaudron wrote: >> >> >> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: >> >>> This commit adds assertions in the functions `shash_count`, >>> `simap_count`, and `smap_count` to ensure that the corresponding input >>> struct pointer is not NULL. >>> >>> This ensures that if the return values of `shash_sort`, `simap_sort`, >>> or `smap_sort` are NULL, then the following for loops would not attempt >>> to access the pointer, which might result in segmentation faults or >>> undefined behavior. >>> >>> Signed-off-by: James Raphael Tiovalen >>> Reviewed-by: Simon Horman >>> --- >>> lib/shash.c | 2 ++ >>> lib/simap.c | 2 ++ >>> lib/smap.c | 1 + >>> 3 files changed, 5 insertions(+) >>> >>> diff --git a/lib/shash.c b/lib/shash.c >>> index a7b2c6458..2bfc8eb50 100644 >>> --- a/lib/shash.c >>> +++ b/lib/shash.c >>> @@ -17,6 +17,7 @@ >>> #include >>> #include "openvswitch/shash.h" >>> #include "hash.h" >>> +#include "util.h" >>> >>> static struct shash_node *shash_find__(const struct shash *, >>> const char *name, size_t name_len, >>> @@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash) >>> size_t >>> shash_count(const struct shash *shash) >>> { >>> +ovs_assert(shash); >> >> My preference would be to return 0, in these instances. What do others think? > > Calling these function with a NULL argument doesn't make much sense to me. > free()-like functions should generally accept NULL pointers, but functions > that actually do work on a datastructure shouldn't, IMO. Fine by me too, so with this: Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 3/8] shash, simap, smap: Add assertions to `*_count` functions
On 7/11/23 12:05, Eelco Chaudron wrote: > > > On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: > >> This commit adds assertions in the functions `shash_count`, >> `simap_count`, and `smap_count` to ensure that the corresponding input >> struct pointer is not NULL. >> >> This ensures that if the return values of `shash_sort`, `simap_sort`, >> or `smap_sort` are NULL, then the following for loops would not attempt >> to access the pointer, which might result in segmentation faults or >> undefined behavior. >> >> Signed-off-by: James Raphael Tiovalen >> Reviewed-by: Simon Horman >> --- >> lib/shash.c | 2 ++ >> lib/simap.c | 2 ++ >> lib/smap.c | 1 + >> 3 files changed, 5 insertions(+) >> >> diff --git a/lib/shash.c b/lib/shash.c >> index a7b2c6458..2bfc8eb50 100644 >> --- a/lib/shash.c >> +++ b/lib/shash.c >> @@ -17,6 +17,7 @@ >> #include >> #include "openvswitch/shash.h" >> #include "hash.h" >> +#include "util.h" >> >> static struct shash_node *shash_find__(const struct shash *, >> const char *name, size_t name_len, >> @@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash) >> size_t >> shash_count(const struct shash *shash) >> { >> +ovs_assert(shash); > > My preference would be to return 0, in these instances. What do others think? Calling these function with a NULL argument doesn't make much sense to me. free()-like functions should generally accept NULL pointers, but functions that actually do work on a datastructure shouldn't, IMO. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 3/8] shash, simap, smap: Add assertions to `*_count` functions
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote: > This commit adds assertions in the functions `shash_count`, > `simap_count`, and `smap_count` to ensure that the corresponding input > struct pointer is not NULL. > > This ensures that if the return values of `shash_sort`, `simap_sort`, > or `smap_sort` are NULL, then the following for loops would not attempt > to access the pointer, which might result in segmentation faults or > undefined behavior. > > Signed-off-by: James Raphael Tiovalen > Reviewed-by: Simon Horman > --- > lib/shash.c | 2 ++ > lib/simap.c | 2 ++ > lib/smap.c | 1 + > 3 files changed, 5 insertions(+) > > diff --git a/lib/shash.c b/lib/shash.c > index a7b2c6458..2bfc8eb50 100644 > --- a/lib/shash.c > +++ b/lib/shash.c > @@ -17,6 +17,7 @@ > #include > #include "openvswitch/shash.h" > #include "hash.h" > +#include "util.h" > > static struct shash_node *shash_find__(const struct shash *, > const char *name, size_t name_len, > @@ -100,6 +101,7 @@ shash_is_empty(const struct shash *shash) > size_t > shash_count(const struct shash *shash) > { > +ovs_assert(shash); My preference would be to return 0, in these instances. What do others think? > return hmap_count(>map); > } > > diff --git a/lib/simap.c b/lib/simap.c > index 0ee08d74d..1c01d4ebe 100644 > --- a/lib/simap.c > +++ b/lib/simap.c > @@ -17,6 +17,7 @@ > #include > #include "simap.h" > #include "hash.h" > +#include "util.h" > > static size_t hash_name(const char *, size_t length); > static struct simap_node *simap_find__(const struct simap *, > @@ -84,6 +85,7 @@ simap_is_empty(const struct simap *simap) > size_t > simap_count(const struct simap *simap) > { > +ovs_assert(simap); > return hmap_count(>map); > } > > diff --git a/lib/smap.c b/lib/smap.c > index 47fb34502..122adca27 100644 > --- a/lib/smap.c > +++ b/lib/smap.c > @@ -300,6 +300,7 @@ smap_is_empty(const struct smap *smap) > size_t > smap_count(const struct smap *smap) > { > +ovs_assert(smap); > return hmap_count(>map); > } > > -- > 2.40.1 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev