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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
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'}}
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400