| Message ID | 20230415152155.762025-4-jamestiotio@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | treewide: Fix multiple Coverity defects | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Sat, Apr 15, 2023 at 11:21:50PM +0800, James Raphael Tiovalen wrote: > This commit adds a check in the functions `shash_count`, `simap_count`, > and `smap_count` to return 0 if the corresponding input struct pointer > is NULL or if the corresponding hmap is empty. > > 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 <jamestiotio@gmail.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 4/15/23 17:21, James Raphael Tiovalen wrote: > This commit adds a check in the functions `shash_count`, `simap_count`, > and `smap_count` to return 0 if the corresponding input struct pointer > is NULL or if the corresponding hmap is empty. > > 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 <jamestiotio@gmail.com> > --- > lib/shash.c | 4 ++++ > lib/simap.c | 4 ++++ > lib/smap.c | 4 ++++ > 3 files changed, 12 insertions(+) > > diff --git a/lib/shash.c b/lib/shash.c > index a7b2c6458..2b1d55901 100644 > --- a/lib/shash.c > +++ b/lib/shash.c > @@ -100,6 +100,10 @@ shash_is_empty(const struct shash *shash) > size_t > shash_count(const struct shash *shash) > { > + if (!shash || shash_is_empty(shash)) { shash_is_empty(shash) is the same as hmap_count(&shash->map) == 0. There should be no need to check, since we're returning the result of hmap_count() below anyway. Same for other structures. And these should, likely, be just assertions. > + return 0; > + } > + > return hmap_count(&shash->map); > } > > diff --git a/lib/simap.c b/lib/simap.c > index 0ee08d74d..48c331dfe 100644 > --- a/lib/simap.c > +++ b/lib/simap.c > @@ -84,6 +84,10 @@ simap_is_empty(const struct simap *simap) > size_t > simap_count(const struct simap *simap) > { > + if (!simap || simap_is_empty(simap)) { > + return 0; > + } > + > return hmap_count(&simap->map); > } > > diff --git a/lib/smap.c b/lib/smap.c > index c1633e2a1..46e87b0ab 100644 > --- a/lib/smap.c > +++ b/lib/smap.c > @@ -300,6 +300,10 @@ smap_is_empty(const struct smap *smap) > size_t > smap_count(const struct smap *smap) > { > + if (!smap || smap_is_empty(smap)) { > + return 0; > + } > + > return hmap_count(&smap->map); > } >
On Tue, Apr 25, 2023 at 4:17 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > shash_is_empty(shash) is the same as hmap_count(&shash->map) == 0. > There should be no need to check, since we're returning the result > of hmap_count() below anyway. > > Same for other structures. And these should, likely, be just assertions. > Got it. In that case, I can just `ovs_assert()` that `shash`, `simap`, and `smap` are not NULL. Best regards, James Raphael Tiovalen
diff --git a/lib/shash.c b/lib/shash.c index a7b2c6458..2b1d55901 100644 --- a/lib/shash.c +++ b/lib/shash.c @@ -100,6 +100,10 @@ shash_is_empty(const struct shash *shash) size_t shash_count(const struct shash *shash) { + if (!shash || shash_is_empty(shash)) { + return 0; + } + return hmap_count(&shash->map); } diff --git a/lib/simap.c b/lib/simap.c index 0ee08d74d..48c331dfe 100644 --- a/lib/simap.c +++ b/lib/simap.c @@ -84,6 +84,10 @@ simap_is_empty(const struct simap *simap) size_t simap_count(const struct simap *simap) { + if (!simap || simap_is_empty(simap)) { + return 0; + } + return hmap_count(&simap->map); } diff --git a/lib/smap.c b/lib/smap.c index c1633e2a1..46e87b0ab 100644 --- a/lib/smap.c +++ b/lib/smap.c @@ -300,6 +300,10 @@ smap_is_empty(const struct smap *smap) size_t smap_count(const struct smap *smap) { + if (!smap || smap_is_empty(smap)) { + return 0; + } + return hmap_count(&smap->map); }
This commit adds a check in the functions `shash_count`, `simap_count`, and `smap_count` to return 0 if the corresponding input struct pointer is NULL or if the corresponding hmap is empty. 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 <jamestiotio@gmail.com> --- lib/shash.c | 4 ++++ lib/simap.c | 4 ++++ lib/smap.c | 4 ++++ 3 files changed, 12 insertions(+)