diff mbox series

[ovs-dev,v12,3/8] shash, simap, smap: Add assertions to `*_count` functions

Message ID 20230613183443.31540-4-jamestiotio@gmail.com
State Accepted, archived
Headers show
Series treewide: Fix multiple Coverity defects | expand

Checks

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

Commit Message

James Raphael Tiovalen June 13, 2023, 6:34 p.m. UTC 
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 <jamestiotio@gmail.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
 lib/shash.c | 2 ++
 lib/simap.c | 2 ++
 lib/smap.c  | 1 +
 3 files changed, 5 insertions(+)

Comments

Eelco Chaudron July 11, 2023, 10:05 a.m. UTC | #1 
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 <jamestiotio@gmail.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> ---
>  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 <config.h>
>  #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(&shash->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 <config.h>
>  #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(&simap->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(&smap->map);
>  }
>
> -- 
> 2.40.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets July 11, 2023, 10:34 p.m. UTC | #2 
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 <jamestiotio@gmail.com>
>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>> ---
>>  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 <config.h>
>>  #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.
Eelco Chaudron July 13, 2023, 12:57 p.m. UTC | #3 
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 <jamestiotio@gmail.com>
>>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>>> ---
>>>  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 <config.h>
>>>  #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 <echaudro@redhat.com>
Ilya Maximets July 13, 2023, 4:15 p.m. UTC | #4 
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 <jamestiotio@gmail.com>
>>>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>>>> ---
>>>>  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 <config.h>
>>>>  #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 <echaudro@redhat.com>
> 

Applied this one as well.  Thanks, everyone!

Will mark remaining 4 patches from this set as 'changes-requested'.

Best regards, Ilya Maximets.
diff mbox series

Patch

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 <config.h>
 #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);
     return hmap_count(&shash->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 <config.h>
 #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(&simap->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(&smap->map);
 }