github / github/codeql

False positive - cpp/constant-comparison

Open
#12,399 2 comments 0 reactions 0 assignees View on GitHub
acknowledged C++ false-positive
Dominant language
CodeQL
Stars
10.1k
Forks
2.1k
Avg merge
2d 15h
Merged PRs (30d)
141

Description

**Description of the false positive**

https://github.com/ryao/zfs/security/code-scanning/275

https://github.com/ryao/zfs/blob/7645b3fc537a7c2960100b3e8da2dec13688b563/lib/libzutil/zutil_import.c#L662-L665

`cpp/constant-comparison` reports `Comparison is always false because c >= 0 and 0 >= children.`, but the second condition is not true. We can set children to a larger value on the line `children = id + 1`. Let us assume that `id` is 0 in this case, such that `children` becomes 1. Immediately afterward, we can run `goto nomem;` following an error from `nvlist_dup()`. Then execution will reach `for (c = 0; c < children; c++)`, where the check suggests that `c < children` is always false, when in reality, it will be true.

Ignoring the false positive, there is another issue with this report. It tells us that `0 >= children` is always true. That could lead one to think that we could have negative values, but `children` is unsigned, so it would have been better to recognize that this is an unsigned type and say `0 == children` is always true. I had to check the type because of that lack of precision, since while `0 >= children` is a valid statement even with unsigned integers, it is not intuitive for a human reading it. Of course, in this situation, the claim that `0 == children` is always true would still be wrong, but it would have made the report slightly less confusing.

That said, I understand that this function is fairly ugly and could stand some refactoring, but CodeQL's query should not be confused by that.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.