[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

The documentation of `ErrnoChecker` is added in the previous change D122150 <https://reviews.llvm.org/D122150>. Really this documentation should be updated when any checker is changed related to errno modeling (errno modeling can be added later to any checker that evaluates standard functions, for example `StreamChecker`). Is it required to mention the `ErrnoModeling` dependency? (The modeling checker is turned on always?) Or add documentation for `ErrnoModeling` too?



================
Comment at: clang/test/Analysis/errno-stdlibraryfunctions-notes.c:15-23
+void test1() {
+  access("path", 0); // no note here
+  access("path", 0);
+  // expected-note at -1{{Assuming that function 'access' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+  if (errno != 0) {
+    // expected-warning at -1{{An undefined value may be read from 'errno'}}
+    // expected-note at -2{{An undefined value may be read from 'errno'}}
----------------
steakhal wrote:
> I must say that I dislike that one cannot disable this warning unless they disable the whole stdlibrarymodeling. It's getting bigger and bigger, thus increasing the value it provides. Yet, not everybody wants to follow SEI CERT guidelines - and I would expect plenty of users in that category, who might be surprised that reading `errno` is a bug.
> 
> Just imagine in the example that between the two `access()` calls, you set `errno` to zero.
> The code would look reasonable.
> 1) reset errno
> 2) do some syscall
> 3) check errno
> 
> I would expect this to be a good practice. We should at least have an option for suppressing reports for such scenarios. IMO it should be rather opt-in, than opt-out, but it might be my personal preference.
> WDYT @martong @xazax.hun 
The CERT rule says that there is no guarantee that `errno` is not changed if the function is successful, regardless if it was 0 or not. There are some functions (that have //in-band return value//) where it is required to set `errno` to 0, there can be another checker (or extend `ErrnoChecker`) to check this.

The new warning is turned on when `alpha.unix.Errno` is turned on, otherwise it should not appear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400