Re: [Qemu-block] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer
On 05/11/18 00:07, Max Reitz wrote:
On 19.10.18 22:39, Liam Merwick wrote:
A NULL 'list' passed into function dump_qlist() isn't correctly
validated and can be passed to qlist_first() where it is dereferenced.
Given that dump_qlist() is static, and callers already do the right
thing, just add an assert to catch future potential bugs (plus the
added benefit of suppressing a warning from a static analysis tool
and removing this noise will help us better find real issues).
But can't you fix the tool?
I don't have access to the tool source but have been filing bugs against
it as I run it on the QEMU codebase and discover false positives.
My opinion is still that large parts of our
code do not assert that some parameter is not NULL, and I think it isn't
a good idea to make them assert that.
Yeah, that can be a slippery slope
I don't know what makes this
function special, and I wonder why it is special to your tool -- as I've
said in the last version, dump_qdict() is basically the same in this
regard. I wonder why your tool doesn't mind that.
I had gone though the code paths to try to see how the tool was happy
with one and not the other - the implementation differed slightly w.r.t
macro usage but I couldn't see any obvious reason.
Can you not whitelist something as false positives? I know we have a
lot of those in Coverity, and we just mark them as such, and that's it.
Yeah, I can flag this as a FP and have it fall off my list.
I'll will drop this patch in v5
Regards,
Liam
Finally, one could argue that the nonnull GCC function attribute would
be a better fit, actually.
But overall, I just don't think it's a good idea to start changing the
code to accommodate for false positives in static analyzers, because in
my experience the number of false positives only rises with time.
Max
Signed-off-by: Liam Merwick
Reviewed-by: Eric Blake
---
block/qapi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/qapi.c b/block/qapi.c
index c66f949db839..e81be604217c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void
*f, int indentation,
const QListEntry *entry;
int i = 0;
+assert(list);
+
for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
QType type = qobject_type(entry->value);
bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
Re: [Qemu-block] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer
On 19.10.18 22:39, Liam Merwick wrote:
> A NULL 'list' passed into function dump_qlist() isn't correctly
> validated and can be passed to qlist_first() where it is dereferenced.
>
> Given that dump_qlist() is static, and callers already do the right
> thing, just add an assert to catch future potential bugs (plus the
> added benefit of suppressing a warning from a static analysis tool
> and removing this noise will help us better find real issues).
But can't you fix the tool? My opinion is still that large parts of our
code do not assert that some parameter is not NULL, and I think it isn't
a good idea to make them assert that. I don't know what makes this
function special, and I wonder why it is special to your tool -- as I've
said in the last version, dump_qdict() is basically the same in this
regard. I wonder why your tool doesn't mind that.
Can you not whitelist something as false positives? I know we have a
lot of those in Coverity, and we just mark them as such, and that's it.
Finally, one could argue that the nonnull GCC function attribute would
be a better fit, actually.
But overall, I just don't think it's a good idea to start changing the
code to accommodate for false positives in static analyzers, because in
my experience the number of false positives only rises with time.
Max
> Signed-off-by: Liam Merwick
> Reviewed-by: Eric Blake
> ---
> block/qapi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index c66f949db839..e81be604217c 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf,
> void *f, int indentation,
> const QListEntry *entry;
> int i = 0;
>
> +assert(list);
> +
> for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
> QType type = qobject_type(entry->value);
> bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
>
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer
A NULL 'list' passed into function dump_qlist() isn't correctly
validated and can be passed to qlist_first() where it is dereferenced.
Given that dump_qlist() is static, and callers already do the right
thing, just add an assert to catch future potential bugs (plus the
added benefit of suppressing a warning from a static analysis tool
and removing this noise will help us better find real issues).
Signed-off-by: Liam Merwick
Reviewed-by: Eric Blake
---
block/qapi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/qapi.c b/block/qapi.c
index c66f949db839..e81be604217c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void
*f, int indentation,
const QListEntry *entry;
int i = 0;
+assert(list);
+
for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
QType type = qobject_type(entry->value);
bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
--
1.8.3.1
