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 <liam.merw...@oracle.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
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);
