[Qemu-devel] [PATCH] util/qemu-option: Document the get_opt_value() function

Philippe Mathieu-Daudé posted 1 patch 4 years, 6 months ago
Test docker-clang@ubuntu failed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Test asan failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190919104907.18005-1-philmd@redhat.com
Maintainers: Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
include/qemu/option.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
[Qemu-devel] [PATCH] util/qemu-option: Document the get_opt_value() function
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago
Coverity noticed commit 950c4e6c94 introduced a dereference before
null check in get_opt_value (CID1391003):

  In get_opt_value: All paths that lead to this null pointer
  comparison already dereference the pointer earlier (CWE-476)

We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
for the 'value' parameter".

Since this function is publicly exposed, it risks new users to do
the same error again. Avoid that documenting the 'value' argument
must not be NULL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/option.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 844587cab3..141d6a883d 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -28,6 +28,18 @@
 
 #include "qemu/queue.h"
 
+/**
+ * get_opt_value
+ * @p: a pointer to the option name, delimited by commas
+ * @value: a non-NULL pointer that will received the delimited options
+ *
+ * The @value char pointer will be allocated and filled with
+ * the delimited options.
+ * It is an error to pass a non-NULL @value parameter.
+ *
+ * Returns the position of the comma delimiter/zero byte after the
+ * option name in @p.
+ */
 const char *get_opt_value(const char *p, char **value);
 
 void parse_option_size(const char *name, const char *value,
-- 
2.20.1


Re: [Qemu-devel] [PATCH] util/qemu-option: Document the get_opt_value() function
Posted by Damien Hedde 4 years, 6 months ago
Hi Philippe,

On 9/19/19 12:49 PM, Philippe Mathieu-Daudé wrote:
> Coverity noticed commit 950c4e6c94 introduced a dereference before
> null check in get_opt_value (CID1391003):
> 
>   In get_opt_value: All paths that lead to this null pointer
>   comparison already dereference the pointer earlier (CWE-476)
> 
> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> for the 'value' parameter".
> 
> Since this function is publicly exposed, it risks new users to do
> the same error again. Avoid that documenting the 'value' argument
> must not be NULL.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/option.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 844587cab3..141d6a883d 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -28,6 +28,18 @@
>  
>  #include "qemu/queue.h"
>  
> +/**
> + * get_opt_value
> + * @p: a pointer to the option name, delimited by commas
> + * @value: a non-NULL pointer that will received the delimited options
> + *
> + * The @value char pointer will be allocated and filled with
> + * the delimited options.
> + * It is an error to pass a non-NULL @value parameter.

You mean "a NULL @value" I suppose (not a non-NULL).

> + *
> + * Returns the position of the comma delimiter/zero byte after the
> + * option name in @p.
> + */
>  const char *get_opt_value(const char *p, char **value);
>  
>  void parse_option_size(const char *name, const char *value,
> 

--
Damien

Re: [Qemu-devel] [PATCH] util/qemu-option: Document the get_opt_value() function
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago
On 9/19/19 12:58 PM, Damien Hedde wrote:
> Hi Philippe,
> 
> On 9/19/19 12:49 PM, Philippe Mathieu-Daudé wrote:
>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>> null check in get_opt_value (CID1391003):
>>
>>   In get_opt_value: All paths that lead to this null pointer
>>   comparison already dereference the pointer earlier (CWE-476)
>>
>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>> for the 'value' parameter".
>>
>> Since this function is publicly exposed, it risks new users to do
>> the same error again. Avoid that documenting the 'value' argument
>> must not be NULL.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/qemu/option.h | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index 844587cab3..141d6a883d 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -28,6 +28,18 @@
>>  
>>  #include "qemu/queue.h"
>>  
>> +/**
>> + * get_opt_value
>> + * @p: a pointer to the option name, delimited by commas
>> + * @value: a non-NULL pointer that will received the delimited options
>> + *
>> + * The @value char pointer will be allocated and filled with
>> + * the delimited options.
>> + * It is an error to pass a non-NULL @value parameter.
> 
> You mean "a NULL @value" I suppose (not a non-NULL).

Oops... Thanks :)

>> + *
>> + * Returns the position of the comma delimiter/zero byte after the
>> + * option name in @p.
>> + */
>>  const char *get_opt_value(const char *p, char **value);
>>  
>>  void parse_option_size(const char *name, const char *value,
>>
> 
> --
> Damien
>