| Message ID | 20200629070858.19850-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] util/qemu-option: Document the get_opt_value() function | expand |
On Mon, Jun 29, 2020 at 09:08:58AM +0200, 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> > --- > v2: Drop confuse comment (Damien Hedde) > --- > include/qemu/option.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
Cc'ing qemu-trivial. On 6/29/20 9:08 AM, 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> > --- > v2: Drop confuse comment (Damien Hedde) > --- > include/qemu/option.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/qemu/option.h b/include/qemu/option.h > index eb4097889d..ac50d25774 100644 > --- a/include/qemu/option.h > +++ b/include/qemu/option.h > @@ -28,6 +28,19 @@ > > #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. > + * > + * Returns the position of the comma delimiter/zero byte after the > + * option name in @p. > + * The memory pointer in @value must be released with a call to g_free() > + * when no longer required. > + */ > const char *get_opt_value(const char *p, char **value); > > void parse_option_size(const char *name, const char *value, >
Le 29/06/2020 à 09:08, Philippe Mathieu-Daudé a écrit : > 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> > --- > v2: Drop confuse comment (Damien Hedde) > --- > include/qemu/option.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/qemu/option.h b/include/qemu/option.h > index eb4097889d..ac50d25774 100644 > --- a/include/qemu/option.h > +++ b/include/qemu/option.h > @@ -28,6 +28,19 @@ > > #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. > + * > + * Returns the position of the comma delimiter/zero byte after the > + * option name in @p. > + * The memory pointer in @value must be released with a call to g_free() > + * when no longer required. > + */ > const char *get_opt_value(const char *p, char **value); > > void parse_option_size(const char *name, const char *value, > Applied to my trivial-patches-for-5.1 branch. Thanks, Laurent
On 6/29/20 12:08 AM, 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. I think we should also add some use of __attribute__((nonnull(...))) to enforce this within the compiler. I recently did this without a qemu/compiler.h QEMU_FOO wrapper within target/arm. But the nonnull option has optional arguments, so it might be difficult to wrap in macros. r~
On 7/7/20 3:14 AM, Richard Henderson wrote: > On 6/29/20 12:08 AM, 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. > > I think we should also add some use of __attribute__((nonnull(...))) to enforce > this within the compiler. > > I recently did this without a qemu/compiler.h QEMU_FOO wrapper within > target/arm. But the nonnull option has optional arguments, so it might be > difficult to wrap in macros. I have this patch after your suggestion from last year: +#if __has_attribute(nonnull) +# define QEMU_NONNULL(LIST) __attribute__((nonnull((LIST)))) +#else +# define QEMU_NONNULL(LIST) +#endif Examples: SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type, - uint32_t id); + uint32_t id) QEMU_NONNULL(1); SpaprDrc *spapr_drc_by_index(uint32_t index); SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id); -int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask); +int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask) + QEMU_NONNULL(3); ... /** * memory_region_init_iommu: Initialize a memory region of a custom type @@ -1066,7 +1073,8 @@ void memory_region_init_iommu(void *_iommu_mr, const char *mrtypename, Object *owner, const char *name, - uint64_t size); + uint64_t size) + QEMU_NONNULL(4); /** * memory_region_init_ram - Initialize RAM memory region. Accesses into the @@ -1154,7 +1162,8 @@ void memory_region_init_rom_device(MemoryRegion *mr, void *opaque, const char *name, uint64_t size, - Error **errp); + Error **errp) + QEMU_NONNULL(2); I can send as RFC is that looks OK to you. Regards, Phil.
Richard Henderson <richard.henderson@linaro.org> writes: > On 6/29/20 12:08 AM, 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. > > I think we should also add some use of __attribute__((nonnull(...))) to enforce > this within the compiler. > > I recently did this without a qemu/compiler.h QEMU_FOO wrapper within > target/arm. But the nonnull option has optional arguments, so it might be > difficult to wrap in macros. Do we support building with a compuler that lacks this attribute?
On 07/07/2020 07.35, Markus Armbruster wrote: > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 6/29/20 12:08 AM, 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. >> >> I think we should also add some use of __attribute__((nonnull(...))) to enforce >> this within the compiler. >> >> I recently did this without a qemu/compiler.h QEMU_FOO wrapper within >> target/arm. But the nonnull option has optional arguments, so it might be >> difficult to wrap in macros. > > Do we support building with a compuler that lacks this attribute? It seems to be available in GCC 4.0 already: https://gcc.gnu.org/onlinedocs/gcc-4.0.0/gcc/Function-Attributes.html ... so the answer to your question is certainly "no". All supported compilers should have this attribute. Thomas
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > 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> > --- > v2: Drop confuse comment (Damien Hedde) > --- > include/qemu/option.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/qemu/option.h b/include/qemu/option.h > index eb4097889d..ac50d25774 100644 > --- a/include/qemu/option.h > +++ b/include/qemu/option.h > @@ -28,6 +28,19 @@ > > #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 s/received/receive/ > + * > + * The @value char pointer will be allocated and filled with > + * the delimited options. > + * > + * Returns the position of the comma delimiter/zero byte after the > + * option name in @p. > + * The memory pointer in @value must be released with a call to g_free() > + * when no longer required. > + */ > const char *get_opt_value(const char *p, char **value); > > void parse_option_size(const char *name, const char *value, You are adding a *second* doc comment: the definition already has one. It's clearer than yours on some things, and less explicit on others. Feel free to improve or replace it. But do put it next to the definition. I'm not trying to re-argue where to put doc comments. I *am* arguing for local consistency while we lack global consistency. For code I maintain, I insist on local consistency. The code belonging to MAINTAINERS section "Command line option argument parsing" has doc comments next to the definition. Except for qemu_opt_has_help_opt(), which predates my maintainer mandate.
On 7/7/20 7:48 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> 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> >> --- >> v2: Drop confuse comment (Damien Hedde) >> --- >> include/qemu/option.h | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/include/qemu/option.h b/include/qemu/option.h >> index eb4097889d..ac50d25774 100644 >> --- a/include/qemu/option.h >> +++ b/include/qemu/option.h >> @@ -28,6 +28,19 @@ >> >> #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 > > s/received/receive/ > >> + * >> + * The @value char pointer will be allocated and filled with >> + * the delimited options. >> + * >> + * Returns the position of the comma delimiter/zero byte after the >> + * option name in @p. >> + * The memory pointer in @value must be released with a call to g_free() >> + * when no longer required. >> + */ >> const char *get_opt_value(const char *p, char **value); >> >> void parse_option_size(const char *name, const char *value, > > You are adding a *second* doc comment: the definition already has one. > It's clearer than yours on some things, and less explicit on others. > Feel free to improve or replace it. But do put it next to the > definition. Hmm I haven't noticed it, because my reflex is to look at the usage description in the prototype declaration, not in the implementation. I know, 2 different schools. Maybe we can make both schools less unhappy by simply duplicating the function description in both the header and the source files... > > I'm not trying to re-argue where to put doc comments. We could, because the origin of both this patch and the commits referenced that produced CID1391003. > I *am* arguing > for local consistency while we lack global consistency. For code I > maintain, I insist on local consistency. > > The code belonging to MAINTAINERS section "Command line option argument > parsing" has doc comments next to the definition. Except for > qemu_opt_has_help_opt(), which predates my maintainer mandate. > Please disregard this patch, I don't mind about get_opt_value().
On Tue, Jul 07, 2020 at 04:14:40AM +0200, Philippe Mathieu-Daudé wrote: > On 7/7/20 3:14 AM, Richard Henderson wrote: > > On 6/29/20 12:08 AM, 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. > > > > I think we should also add some use of __attribute__((nonnull(...))) to enforce > > this within the compiler. > > > > I recently did this without a qemu/compiler.h QEMU_FOO wrapper within > > target/arm. But the nonnull option has optional arguments, so it might be > > difficult to wrap in macros. > > I have this patch after your suggestion from last year: > > +#if __has_attribute(nonnull) > +# define QEMU_NONNULL(LIST) __attribute__((nonnull((LIST)))) > +#else > +# define QEMU_NONNULL(LIST) > +#endif The if/else branch is not required, as both clang and gcc support this, and they are our only supported compilers. Beware that __attribute__((nonnul)) has side-effects, as it was originally implemented as a hint for the optimizer. It allows it to eliminate any code in the method that does a comparison to NULL. Historically it only generated warning messages in very few scenarios involving a literal NULL. Only more recently with -fanalyzer can it generate warnings about indirect passing of NULL via variables. Regards, Daniel
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 7/7/20 7:48 AM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: >> >>> 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> >>> --- >>> v2: Drop confuse comment (Damien Hedde) >>> --- >>> include/qemu/option.h | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/include/qemu/option.h b/include/qemu/option.h >>> index eb4097889d..ac50d25774 100644 >>> --- a/include/qemu/option.h >>> +++ b/include/qemu/option.h >>> @@ -28,6 +28,19 @@ >>> >>> #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 >> >> s/received/receive/ >> >>> + * >>> + * The @value char pointer will be allocated and filled with >>> + * the delimited options. >>> + * >>> + * Returns the position of the comma delimiter/zero byte after the >>> + * option name in @p. >>> + * The memory pointer in @value must be released with a call to g_free() >>> + * when no longer required. >>> + */ >>> const char *get_opt_value(const char *p, char **value); >>> >>> void parse_option_size(const char *name, const char *value, >> >> You are adding a *second* doc comment: the definition already has one. >> It's clearer than yours on some things, and less explicit on others. >> Feel free to improve or replace it. But do put it next to the >> definition. > > Hmm I haven't noticed it, because my reflex is to look at the usage > description in the prototype declaration, not in the implementation. > > I know, 2 different schools. > > Maybe we can make both schools less unhappy by simply duplicating the > function description in both the header and the source files... Worst of both worlds :) [...]
Thomas Huth <thuth@redhat.com> writes: > On 07/07/2020 07.35, Markus Armbruster wrote: >> Richard Henderson <richard.henderson@linaro.org> writes: >> >>> On 6/29/20 12:08 AM, 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. >>> >>> I think we should also add some use of __attribute__((nonnull(...))) to enforce >>> this within the compiler. >>> >>> I recently did this without a qemu/compiler.h QEMU_FOO wrapper within >>> target/arm. But the nonnull option has optional arguments, so it might be >>> difficult to wrap in macros. >> >> Do we support building with a compuler that lacks this attribute? > > It seems to be available in GCC 4.0 already: > > https://gcc.gnu.org/onlinedocs/gcc-4.0.0/gcc/Function-Attributes.html > > ... so the answer to your question is certainly "no". All supported > compilers should have this attribute. Why a macro then?
diff --git a/include/qemu/option.h b/include/qemu/option.h index eb4097889d..ac50d25774 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -28,6 +28,19 @@ #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. + * + * Returns the position of the comma delimiter/zero byte after the + * option name in @p. + * The memory pointer in @value must be released with a call to g_free() + * when no longer required. + */ const char *get_opt_value(const char *p, char **value); void parse_option_size(const char *name, const char *value,
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> --- v2: Drop confuse comment (Damien Hedde) --- include/qemu/option.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)