The case of NULL stream passed to stream functions was reported by StreamChecker.
The same condition is checked already by StdLibraryFunctionsChecker and it is
enough to check at one place. The StreamChecker stops now analysis if a passed NULL
stream is encountered but generates no report.
This change removes a dependency between StdCLibraryFunctionArgs checker and
StreamChecker. There is now no more specific message reported by StreamChecker,
the previous weak-dependency is not needed. And StreamChecker can be used
without StdCLibraryFunctions checker or its ModelPOSIX option.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Test file std-c-library-functions-vs-stream-checker.c could be removed. It tests the same as the newly added stream-stdlibraryfunctionargs.c.
IIRC we talked about it would only really make sense to evaluate this patch stack as a whole, not piece by piece, but I'm not seeing results on open source projects here either. Can you please post them?
Some reports can be found here (if the link works and the data does not expire), the runs stored on 2022-12-09.
Results appeared only projects "postgres" and "curl" (from memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,libwebm) from checkers StdCLibraryFunctionArgs and Errno.
On the postgres results, the second is one that can be fixed in the checker (add special cases to StdLibraryFunctionsChecker for zero len or size fread and fwrite arguments). The others are false positives because the error path is impossible because implicit constraints (what is not known to the analyzer) on variables.
These curl results look faulty, the last fclose call looks not recognized.
Please make the link a little more reviewer friendly. You can make a CodeChecker URL where the appropriate runs are already selected, with the appropriate checkers being filtered, and uniquing turned on, like this:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=libwebm_libwebm-1.0.0.27_baseline&run=postgres_REL_13_0_baseline&run=ffmpeg_n4.3.1_baseline&run=sqlite_version-3.33.0_baseline&run=openssl_openssl-3.0.0-alpha7_baseline&run=vim_v8.2.1920_baseline&run=twin_v0.8.1_baseline&run=curl_curl-7_66_0_baseline&run=tmux_2.6_baseline&run=memcached_1.6.8_baseline&is-unique=on&diff-type=New&checker-name=alpha.unix.Errno&checker-name=alpha.unix.StdCLibraryFunctionArgs
Also, I'd appreciate links to the specific reports you are describing. I'm not sure what postgres results you are talking about, because I suspect I listed them differently. You made a link to the curl result, so please do it for the others as well.
I'd want some more thorough explanations as well. Path events are numbered in CodeChecker, you can use them to explain why you think the report is a false positive.
Here is a postgers result link, results are from StdCLibraryFunctionArgs checker. The second report is in file relcache.c (it looks like that data pointer is really null but the len value is 0, and in this special case the data pointer should be ignored according to the standards). A FIXME for this case was added to the code in D135247.
About the first postgres result in pg_backup_tar.c:
At step 3 we see that original value of len is 1. The condition in step 5 is true, then th->filelen-th->pos should be < 1. This is the new value assigned to len. In step 6 len can be only <=0. Still the condition is assumed to be false what is not possible. At this point the path becomes already invalid.
I have a fear that we may have too few results as well -- maybe we should expand our testing infrastructure with more POSIX API heavy codebases. Looking at a few new projects, https://github.com/audacity/audacity looks like a good candidate, but of course it often turns out that adding a new project to the benchmark is more troublesome than it appears...
Could you please re-evaluate the patch stack with the new fix?
edit: Sorry, realized you said FIXME, not fix :)
I would split the patches D135360 and D135247 (the two before this) into new patches, one for StreamChecker, other for StdLibraryFunctionsChecker only. This can make review (and commit history) more clear. And StdLibraryFunctionsChecker can be modified probably in a new patch that comes before these, to change how NoteTags are added.
Are you sure that the refactoring made no changes to the results? Could you maybe just run a nightly or something like that to confirm?
Please rerun the evaluation before commiting to confirm the results haven't changed! Otherwise, LGTM.
clang/test/Analysis/stream.c | ||
---|---|---|
89 |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
507–511 | Okay, but how does CheckerContext::isDifferent() misbehave in that case? |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
507–511 | I do not know exactly why I put this this FIXME here. I was probably thinking about a situation when errno does not change in any case and the modeled function (feof) has no other effect. But feof should not change errno only if the stream is valid, and if the stream is unknown it may be invalid. Now the evalCall returns false, a default evaluation follows and errno is invalidated, this is how it should work. The FIXME can be removed. |
Okay, but how does CheckerContext::isDifferent() misbehave in that case?