cpp/uninitialized-local - false positive

[ryao]

Description of the false positive

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

https://github.com/ryao/zfs/blob/15e39c6d547afb7e8df515582516f318dd5d0183/lib/libzpool/kernel.c#L1205-L1205

CodeQL complains that done is potentially uninitialized. However, done is set when rc != -1, so it is only uninitialized when rc == -1. Then the function terminates when rc < 0 before reaching the line where done is used. There is no path that can reach that line that leaves done uninitialized. Therefore, this is a false positive.

[aeisenberg]

Thanks for your false positive report. It's likely that codeql is not catching that pwrite can only return values >= -1. Would you be able to try changing if (rc != -1) { to if (rc > -1) { or change if (rc < 0) to if (rc == -1).

[ryao]

Thanks for your false positive report. It's likely that codeql is not catching that pwrite can only return values >= -1.

While CodeQL might not realize that pwrite64() can only return values >= -1, that should not be a factor in this specific instance. If we assume that pwrite64() can return values < -1, then whenever it does return such a value or more generally, whenever it returns a negative number, if (rc < 0) return (errno); will prevent execution from reaching done += rc;. Not knowing that pwrite64() cannot have negative return values below -1 should not be relevant here.

As an off-topic remark, I have never known whether I should write code that checks for rc == -1 or rc < 0 because they are both "correct". However, this issue has made me realize that rc < 0 is superior because it ignores the need for static analyzers to understand the return values of the underlying C library functions. I will be using it from now on. :)

Would you be able to try changing if (rc != -1) { to if (rc > -1) { or change if (rc < 0) to if (rc == -1).

The latter should permit a false positive if CodeQL does not know that pwrite64() cannot return negative numbers below -1 and would make the complaint semi-valid (in the situation that the kernel underneath us has a bug).

The former seems unlikely to make a difference. I have pushed it to the sa branch in ryao/zfs. We will have results in less than a half hour.

[ryao]

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

It is still detected with that change. See the sa branch to see the commit:

openzfs/zfs@master...ryao:zfs:sa

[aeisenberg]

Thanks for trying and thanks for your feedback. I have forwarded this issue on to the C++ team and they will be able to get back to you.

[aeisenberg]

Related issue: #11216

[MathiasVP]

Hi @ryao,

Thanks for raising this issue. Indeed, this is false positive. The underlying reason for the false positive is that the query is unable to deduce that the "context" in which the initialization of done is skipped (i.e., when rc <= -1) implies that control cannot reach the done += rc; operation (as reaching this operation would imply that rc >= 0).

I'll create an internal issue for this. I can't quite give an estimate for when FPs like this has been fixed (since this is a rather hard topic to tackle in sufficient generality), but I'll be sure to let you know once we make progress in this area!

[MathiasVP]

This has been fixed in #14704. The query now only reports an alert if the use is always uninitialized. This results in a vast reduction in the number of FPs for the query.