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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

It looks everything fine, given that the `alpha.unix.Errno` will depend on the `apimodeling.StdCLibraryFunctions`.
This should do the expected errno state propagation by which the `Errno` checker can report diagnostics - WHILE the `StdCLibraryFunctions` still won't report diagnostics.
This means that the Errno part /diagnostics can be separately disabled from the diagnostics of `StdCLibraryFunctionArgsChecker` and other frontend checkers of the `StdCLibraryFunctionsChecker` modeling part.
Excellent.

Please demonstrate that indeed disabling separately `Errno` or `StdCLibraryFunctionArgsChecker` will make the respective reports disappear.

---

I can see that in many cases you introduced an additional `.Case()`, which will result in a state-split.
I'm curious what runtime implications this change introduces. Please measure this.

I must also admit that I haven't checked the parameters of `Case()` in all cases; so I would encourage you to double-check those to make sure they are in line with the API specification.

Given these changes are implemented, I think it's ready to land. Awesome job, really!
Sorry about reject, I just saw it's green and I still had some thoughts about this.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:68-69
+    return "may be undefined after the call and should not be used";
+  };
+}
+
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:444
+      DefinedOrUnknownSVal Cond =
+          SVB.evalBinOpNN(State, Op, ErrnoSVal, ZeroVal, SVB.getConditionType())
+              .castAs<DefinedOrUnknownSVal>();
----------------
nit: we should not use the `NN` and other suffixed versions of this API. The generic one should suffice.
Actually, I plan to refactor all code and make those private.


================
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'}}
----------------
martong wrote:
> balazske wrote:
> > 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.
> > I must say that I dislike that one cannot disable this warning unless they disable the whole stdlibrarymodeling. 
> 
> Yeah, good point. Perhaps we should have a new checker option that enables/disables errno related reports. Should this be part of the ErrnoChecker (or the StdLibraryFunctionsChecker)?
> The new warning is turned on when alpha.unix.Errno is turned on, otherwise it should not appear.
Oh, I see. I might overreacted this part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400