github / github/codeql

False positive - cpp/constant-comparison

Aberta
#12,399 2 comentários 0 reações 0 responsáveis Ver no GitHub
acknowledged C++ false-positive
Linguagem predominante
CodeQL
Estrelas
10.1k
Forks
2.1k
Merge médio
2d 15h
PRs com merge (30d)
141

Descrição

**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.

Guia de contribuição

Abrir o guia de contribuição

Avaliação

Esta issue ainda não foi avaliada.

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.