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@-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@-1{{An undefined value may be read from 'errno'}}
+ // expected-note@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
