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

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

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

It's getting in really good shape. Nice work.
I've got some nits about docs and checker dependencies and also a major comment inline.

The documentation needs to be updated to keep it in sync.
It should demonstrate the new warnings, state why we have them. Remember to refer back to the CERT rule for additional resources.
Refer to the related checker, in this case to `ErrnoModeling`, and explain how it changes the analysis.
Shouldn't it be a modeling dependency rather?



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400