Open ryao opened 1 year ago
Another instance of this is here:
https://github.com/ryao/zfs/security/code-scanning/424
Another instance that was reported twice:
https://github.com/ryao/zfs/security/code-scanning/425 https://github.com/ryao/zfs/security/code-scanning/426
The following all complain about the same fscanf()
, although a false positive on this one is somewhat understandable since the code is designed to rely on a default that remains should the fscanf()
call fail:
https://github.com/ryao/zfs/security/code-scanning/421 https://github.com/ryao/zfs/security/code-scanning/422 https://github.com/ryao/zfs/security/code-scanning/423
That one depends entirely on programmer intent, so I am not sure what could be done about that. Maybe we could use (void)
before fscanf()
to indicate that we do not care about the operation's success when the variable is already initialized. However, I vaguely recall the way the code is written was done intentionally to work around a compiler warning that tripped -Wall -Werror
. It was also caught by cpp/empty-if
and a past attempt to clean it up tripped a bug in GCC's diagnostics. :/
Hi @ryao, thanks a lot for this report. I can confirm these findings. It seems like you found two different sources of false positives:
scanf
-like destination in the failing branch of the call do not seem to sanitize the unsafe undeclared variablescanf
analysis seems to not work very well with static variables. In particular it seems to ignore that those are always 0-initialised, which seems to throw off the analysis for alerts like https://github.com/ryao/zfs/security/code-scanning/421.I've added these false positives to our tests, but at this time I cannot give an estimate as to when we will be able to fix this.
https://github.com/ryao/zfs/security/code-scanning/420
https://github.com/ryao/zfs/blob/3881dd42bbfb7297f08e796c38b35d54e11ac500/lib/libspl/os/linux/gethostid.c#L50
CodeQL says
This variable is read, but may not have been written. It should be guarded by a check that the returns at least 1.
. However, that is already being done as part ofif (fscanf(f, "%lx", &hostid) != 1)
.