[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

Fri Jun 9 03:42:10 PDT 2023

Szelethus added a comment.

> I am not sure about the exact requirements, this review can be a place for discussion about what should be fixed (if any).

D52984 <https://reviews.llvm.org/D52984> added the "Making your checker better" section to the dev manual: https://clang-analyzer.llvm.org/checker_dev_manual.html (nobody can be faulted for not finding this, aside from those that witnessed its creation, it has faded from the collective memory of the analyzer developers).

In D152436#4405558 <https://reviews.llvm.org/D152436#4405558>, @balazske wrote:

> I could test the checker on these projects (CTU analysis was not used):
> memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,tinyxml2,libwebm,xerces,bitcoin,protobuf,qtbase,contour,acid
>
> These are reports that could be improved:
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=memcached_1.6.8_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=2bf08110160cdf74b43d1443a243c170&report-filepath=%2aauthfile.c&report-id=1930445>
> In this case function `fileno` returns -1 because of failure, but this is not indicated in a `NoteTag`. This is a correct result, only the note is missing. This problem can be solved if a note is displayed on every branch ("case") of the standard C functions. But this leads to many notes at un-interesting places. If the note is displayed only at "interesting" values another difficulty shows up: The note disappears from places where it should be shown because the "interestingness" is not set, for example at conditions of `if` statement. So the solution may require more work. This case with function `fileno` occurs 13 times in all the tested projects.

Yeah, this is a tough cookie... is it okay to find hide the -1 branch behind an off-by-default checker option for the time being?

> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=e7c81b8628bb636f9087a14b446dcb00&report-id=1930482&report-filepath=%2aclient.c>
> The function `open` is not modeled in `StdCLibraryFunctionsChecker`, it should not return less than -1 but this information is not included now.
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=twin_v0.8.1_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=ed57f448095d9ba67d047a45898d1ebb&report-id=1930547&report-filepath=%2alibTw.c>
> `socket` can not return less than -1 but this function is not modeled currently.

These should be a rather painless fix, right?

> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=twin_v0.8.1_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=50a98122502701302b7b75a6a56342e8&report-id=1930627&report-filepath=%2ashm.c>
> This looks wrong, `L` should not be 0 because `len` looks > 0 (see the macros that set `len`). Probably the many bitwise operations cause the problem.
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=bitcoin_v0.20.1_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=6ad3a20f18f2850293b4cdd867e404e2&report-id=1932643&report-filepath=%2aenv_posix.cc>
> When `file_size` is 0 `status.ok()` is probably false that is not correctly recognized (may work in CTU mode?).

Looks like something we can live with.

> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclibraryfunctions_test&is-unique=on&page=2&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=ebc6fce212f7238143e8f97b1d231abf&report-id=1932035&report-filepath=%2arelcache.c>
> `fwrite` with 0 buffer and 0 size should not be an error, this is not checked now.

Some discussion for that: D140387#inline-1360054 <https://reviews.llvm.org/D140387#inline-1360054>. There is a FIXME in the code for it -- not sure how common this specific issue is, but we did stumble on it in an open source project... how painful would it be to fix this?

> These results look good:
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=curl_curl-7_66_0_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=710c0fca72ffec642cbcfcc8311a8fbd&report-id=1930510&report-filepath=%2asockfilt.c>
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=curl_curl-7_66_0_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=d4a4bda38c5a6fdaabe2c1867158b106&report-id=1930493&report-filepath=%2atftpd.c>
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=vim_v8.2.1920_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=e15776574d709197d3c07be5bcb7ae40&report-id=1930738&report-filepath=%2aos_unix.c>
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=ffmpeg_n4.3.1_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=908f965d980d60292af95db0fa10cd5f&report-id=1931604&report-filepath=%2av4l2_buffers.c>
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=1928ba718d9742340937d425ec3978c6&report-id=1932337&report-filepath=%2apg_backup_custom.c>
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=58d8278be40f99597b44323d2574c053&report-id=1932391&report-filepath=%2asyslogger.c>
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=914e79646cb0de40dab434ba24c8c23c&report-id=1932141&report-filepath=%2adsm_impl.c>
> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=xerces_v3.2.3_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=4ab640064066880ac7031727869c92f4&report-id=1932564&report-filepath=%2aThreadTest.cpp>
> In this last case it looks like that previous call to `ftell` returns -1, this value is assigned to `fileSize`. This is again a case for improvement similar as the case with `fileno`.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152436/new/

https://reviews.llvm.org/D152436