[PATCH] D122150: [clang][analyzer] Add checker for bad use of 'errno'.

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

steakhal added a comment.

I believe `evalAssume` would be a better fit for diagnosing such issues, but I can see your point that we don't have `CheckerContext` there to emit reports.
That being said, the `check::Location` is the best alternative.

Please also check the not `done` comments.



================
Comment at: clang/docs/analyzer/checkers.rst:2529
+Check for unsafe use of ``errno``.
+This checker can find the first read of ``errno`` after not failed standard
+function calls.
----------------
successful


================
Comment at: clang/docs/analyzer/checkers.rst:2538-2539
+yet supported by the checker.
+The return values for the failure cases are documented in the standard Linux man
+pages of the functions.
+
----------------
Maybe place a crossref for this.

Also, am I right that there is a CERT rule about this issue?
We should probably mention that, while placing a crossref too.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:550
+def ErrnoChecker : Checker<"Errno">,
+  HelpText<"Check not recommended uses of 'errno'">,
+  Dependencies<[ErrnoModeling]>,
----------------
antipatterns?


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:554
+    CmdLineOption<Boolean,
+                  "AllowUnconditionalErrnoRead",
+                  "Allow read of undefined value from errno outside of conditions",
----------------
My problem with this name is that `Unconditional` seems to bind to the action, not to the place where the read happens (inside condition expressions).
I find this pretty confusing.

How about calling this `AllowReadingErrnoInsideConditionExpressions`. Still bad, but probably better.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:558
+                  InAlpha,
+                  Hide>, // ??
+  ]>,
----------------
balazske wrote:
> steakhal wrote:
> > I think we have 'Hidden', but not 'Hide'.
> > It means that it is a modeling checker. Any checker that emits diagnostics, should **not** be marked 'Hidden'.
> This "Hide" applies for the option, not for the checker. It was copied from another place. I am not sure if it must be used here.
Quote `CheckerBase.td:39`
```
/// Marks the entry hidden. Hidden entries won't be displayed in
/// -analyzer-checker-option-help.
class HiddenEnum<bit val> {
  bit Val = val;
}
def DontHide : HiddenEnum<0>;
def Hide : HiddenEnum<1>;
```

I believe, we should expose this option. Otherwise why would we introduce it in the first place?
The default value of this is actually `DontHide` in the `CmdLineOption<>` td class. So, you should be fine by simply omitting this keyword here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:10-11
+// This defines an "errno checker" that can detect some invalid use of the
+// system-defined value 'errno'. This checker works together with the
+// ErrnoModeling checker.
+//
----------------
and the StdLibraryFunctionsChecker per D125400.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:101-102
+      break;
+    default:
+      break;
+    }
----------------
We should also handle `BinaryOperator` for conditional operators (EQ, NE, LT, LE, ...). Or at least leave a FIXME about this.
Have a test demonstrating the behavior.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:261
+  if (unsigned EState = State->get<ErrnoState>())
+    return static_cast<ErrnoCheckState>(EState);
+  return Errno_Irrelevant;
----------------
I believe you no longer need to cast enums to numbers and back for enums and other types.
`get<ErrnoState>()` should return the right type.
See D126801.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:280
+      BR.markNotInteresting(ErrnoR);
+      return Message.c_str();
+    }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122150