[PATCH] D44143: Create properly seeded random generator check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this check!

Do you think it might be possible to also add a check for `cert-msc32-c` that handles the C side of things, and use a common check to implement both? C won't ever have the C++'isms, but C++ can certainly abuse `std::srand()` so it seems like there's at least some overlap.



================
Comment at: clang-tidy/cert/CERTTidyModule.cpp:43
+    CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
+        "cert-properly-seeded-random-generator");
     CheckFactories.registerCheck<VariadicFunctionDefCheck>("cert-dcl50-cpp");
----------------
The name should be `cert-msc51-cpp` (and categorized with the other msc checks).


================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:22-24
+      hasAnyName("linear_congruential_engine", "mersenne_twister_engine",
+                 "subtract_with_carry_engine", "discard_block_engine",
+                 "independent_bits_engine", "shuffle_order_engine"));
----------------
These should be using fully-qualified names, like `::std::mersenne_twister_engine`. Also, I think this list should be configurable so that users can add their own engines if needed.


================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:66
+
+  template<class T>
+void ProperlySeededRandomGeneratorCheck::checkSeed(
----------------
Formatting looks off here.


================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:71
+    diag(Func->getExprLoc(), "random number generator must be seeded with "
+                             "a random_device instead of default argument");
+    return;
----------------
The diagnostics here aren't quite correct -- a `random_device` isn't *required*. For instance, it's also fine to open up /dev/random and read a seed's worth of bytes. I think a better phrasing might be: `random number generator seeded with a default argument will generate a predictable sequence of values`.


================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:77-78
+  if (Func->getArg(0)->EvaluateAsInt(Value, *Result.Context)) {
+    diag(Func->getExprLoc(), "random number generator must be seeded with "
+                             "a random_device instead of a constant");
+    return;
----------------
I'd probably use similar wording here: `random number generator seeded with a constant value will generate a predictable sequence of values`.

You can combine these diagnostics and use %select{}0 to choose between the variants.


================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.h:33-34
+  template<class T>
+  void checkSeed(const ast_matchers::MatchFinder::MatchResult &Result,
+                 const T *Func);
+};
----------------
It seems like this should be a private function rather than public.


================
Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:3
+
+cert-properly-seeded-random-generator
+=====================================
----------------
When renaming the check, be sure to update titles, file names, etc.


================
Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:7
+This check flags all pseudo-random number engines and engine adaptors
+instantiations when it initialized or seeded with default argument or constant
+expression. Pseudo-random number engines seeded with a predictable value may
----------------
Remove "it".


================
Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:22-23
+    
+    std::time_t t;
+    engine1.seed(std::time(&t)); // Bad, system time might be controlled by user
+
----------------
There's no test case for this test, and I don't think this code would generate a diagnostic in the current check form. This might require a (user-configurable) list of disallowed sources of seed values. For instance, it should diagnose when called with a `time_t` value or a direct call to a time function, but it need not diagnose if the value is somehow obscured. e.g.,
```
unsigned get_seed() { std::time_t t; return std::time(&t); }
engine1.seed(get_seed()); // No diagnostic required
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44143