Merge pull request #39576 from jbytheway/detect_bad_rng_seeding

Add clang-tidy check to catch incorrect use of RNGs

src/activity_item_handling.cpp

@@ -1141,11 +1141,11 @@ static activity_reason_info find_base_construction(
 static std::string random_string( size_t length )
 {
     auto randchar = []() -> char {
-        const char charset[] =
+        static constexpr char charset[] =
         "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
         "abcdefghijklmnopqrstuvwxyz";
-        const size_t max_index = ( sizeof( charset ) - 1 );
-        return charset[rand() % max_index];
+        static constexpr size_t num_chars = sizeof( charset ) - 1;
+        return charset[rng( 0, num_chars - 1 )];
     };
     std::string str( length, 0 );
     std::generate_n( str.begin(), length, randchar );

src/faction_camp.cpp

@@ -1838,7 +1838,7 @@ void basecamp::job_assignment_ui()
                 selection--;
             }
         } else if( action == "CONFIRM" ) {
-            if( !stationed_npcs.empty() ) {
+            if( cur_npc ) {
                 while( true ) {
                     uilist smenu;
                     smenu.text = _( "Assign job priority ( 0 to disable )" );

src/item.cpp

@@ -4079,8 +4079,7 @@ void item::handle_pickup_ownership( Character &c )
                     }
                 }
                 if( !guard_chosen ) {
-                    int random_index = rand() % witnesses.size();
-                    witnesses[random_index]->witness_thievery( &*this );
+                    random_entry( witnesses )->witness_thievery( &*this );
                 }
             }
             set_owner( c );

src/rng.cpp

@@ -123,6 +123,7 @@ double rng_normal( double lo, double hi )
 
 cata_default_random_engine &rng_get_engine()
 {
+    // NOLINTNEXTLINE(cata-determinism)
     static cata_default_random_engine eng(
         std::chrono::high_resolution_clock::now().time_since_epoch().count() );
     return eng;

src/sdlsound.cpp

@@ -206,6 +206,9 @@ void play_music( const std::string &playlist )
         playlist_indexes.push_back( i );
     }
     if( list.shuffle ) {
+        // Son't need to worry about the determinism check here because it only
+        // affects audio, not game logic.
+        // NOLINTNEXTLINE(cata-determinism)
         static auto eng = cata_default_random_engine(
                               std::chrono::system_clock::now().time_since_epoch().count() );
         std::shuffle( playlist_indexes.begin(), playlist_indexes.end(), eng );

src/text_snippets.cpp

@@ -158,7 +158,9 @@ cata::optional<translation> snippet_library::random_from_category( const std::st
         return cata::nullopt;
     }
     const size_t count = it->second.ids.size() + it->second.no_id.size();
-    std::mt19937 generator( seed );
+    // This engine is deterministcally seeded, so acceptable.
+    // NOLINTNEXTLINE(cata-determinism)
+    cata_default_random_engine generator( seed );
     std::uniform_int_distribution<size_t> dis( 0, count - 1 );
     const size_t index = dis( generator );
     if( index < it->second.ids.size() ) {

src/weather_gen.cpp

@@ -228,8 +228,7 @@ weather_type weather_generator::get_weather_conditions( const w_point &w ) const
 
 int weather_generator::get_wind_direction( const season_type season ) const
 {
-    unsigned dirseed = std::chrono::system_clock::now().time_since_epoch().count();
-    cata_default_random_engine wind_dir_gen( dirseed );
+    cata_default_random_engine &wind_dir_gen = rng_get_engine();
     // Assign chance to angle direction
     if( season == SPRING ) {
         std::discrete_distribution<int> distribution {3, 3, 5, 8, 11, 10, 5, 2, 5, 6, 6, 5, 8, 10, 8, 6};

tests/math_functions_test.cpp

@@ -4,6 +4,7 @@
 
 #include "catch/catch.hpp"
 #include "cata_utility.h"
+#include "rng.h"
 
 TEST_CASE( "fast_floor", "[math]" )
 {
@@ -23,12 +24,8 @@ TEST_CASE( "fast_floor", "[math]" )
     REQUIRE( fast_floor( 2.1 ) ==  2 );
     REQUIRE( fast_floor( 2.9 ) ==  2 );
 
-    std::random_device rd;
-    std::mt19937 mt( rd() );
-    std::uniform_real_distribution<double> dist( -10.0, 10.0 );
-
     for( size_t i = 0; i != 1000; ++i ) {
-        double val = dist( mt );
+        double val = rng_float( -10, 10 );
         REQUIRE( fast_floor( val ) == std::floor( val ) );
     }
 }

tools/clang-tidy-plugin/CMakeLists.txt

@@ -6,6 +6,7 @@ find_package(Clang REQUIRED CONFIG)
 add_library(
     CataAnalyzerPlugin MODULE
     CataTidyModule.cpp
+    DeterminismCheck.cpp
     HeaderGuardCheck.cpp
     JsonTranslationInputCheck.cpp
     NoLongCheck.cpp

tools/clang-tidy-plugin/CataTidyModule.cpp

@@ -2,6 +2,7 @@
 
 #include "ClangTidyModule.h"
 #include "ClangTidyModuleRegistry.h"
+#include "DeterminismCheck.h"
 #include "HeaderGuardCheck.h"
 #include "JsonTranslationInputCheck.h"
 #include "NoLongCheck.h"
@@ -27,6 +28,7 @@ class CataModule : public ClangTidyModule
 {
     public:
         void addCheckFactories( ClangTidyCheckFactories &CheckFactories ) override {
+            CheckFactories.registerCheck<DeterminismCheck>( "cata-determinism" );
             CheckFactories.registerCheck<CataHeaderGuardCheck>( "cata-header-guard" );
             CheckFactories.registerCheck<JsonTranslationInputCheck>( "cata-json-translation-input" );
             CheckFactories.registerCheck<NoLongCheck>( "cata-no-long" );

tools/clang-tidy-plugin/DeterminismCheck.cpp

@@ -0,0 +1,98 @@
+#include "DeterminismCheck.h"
+
+#include <clang/AST/Decl.h>
+#include <clang/AST/DeclCXX.h>
+#include <clang/AST/Expr.h>
+#include <clang/AST/Type.h>
+#include <clang/ASTMatchers/ASTMatchFinder.h>
+#include <clang/ASTMatchers/ASTMatchers.h>
+#include <clang/ASTMatchers/ASTMatchersInternal.h>
+#include <clang/Basic/Diagnostic.h>
+
+using namespace clang::ast_matchers;
+
+namespace clang
+{
+namespace tidy
+{
+namespace cata
+{
+void DeterminismCheck::registerMatchers( MatchFinder *Finder )
+{
+    Finder->addMatcher(
+        callExpr(
+            callee(
+                functionDecl(
+                    anyOf( hasName( "::rand" ), hasName( "::random" ), hasName( "::drand48" ),
+                           hasName( "::erand48" ), hasName( "::lrand48" ), hasName( "::mrand48" ),
+                           hasName( "::nrand48" ), hasName( "::jrand48" ) )
+                )
+            )
+        ).bind( "call" ), this );
+
+    // As a heuristic we guess that any record type with a "seed" member
+    // function is a random engine.  Later in the code we also check for
+    // "result_type".
+    using TypeMatcher = clang::ast_matchers::internal::Matcher<Decl>;
+    const TypeMatcher IsRngCoreDecl = cxxRecordDecl( hasMethod( hasName( "seed" ) ) );
+    const TypeMatcher IsRngTypedefDecl =
+        typedefNameDecl( hasType( qualType( hasDeclaration( IsRngCoreDecl ) ) ) );
+    const TypeMatcher IsRngTypedef2Decl =
+        typedefNameDecl( hasType( qualType( hasDeclaration( IsRngTypedefDecl ) ) ) );
+    const TypeMatcher IsRngDecl = anyOf( IsRngCoreDecl, IsRngTypedefDecl, IsRngTypedef2Decl );
+    Finder->addMatcher( cxxConstructExpr( hasType( IsRngDecl ) ).bind( "init" ), this );
+}
+
+static void CheckCall( DeterminismCheck &Check, const MatchFinder::MatchResult &Result )
+{
+    const CallExpr *Call = Result.Nodes.getNodeAs<clang::CallExpr>( "call" );
+    if( !Call ) {
+        return;
+    }
+    const NamedDecl *Function = dyn_cast<NamedDecl>( Call->getCalleeDecl() );
+    if( !Function ) {
+        return;
+    }
+    Check.diag(
+        Call->getBeginLoc(),
+        "Call to library random function %0.  To ensure determinism for a fixed seed, "
+        "use the common tools from rng.h rather than calling library random functions." )
+            << Function;
+}
+
+static void CheckInit( DeterminismCheck &Check, const MatchFinder::MatchResult &Result )
+{
+    const CXXConstructExpr *Construction = Result.Nodes.getNodeAs<CXXConstructExpr>( "init" );
+    if( !Construction ) {
+        return;
+    }
+    const CXXRecordDecl *ConstructedTypeDecl = Construction->getConstructor()->getParent();
+    bool FoundResultType = false;
+    for( const Decl *D : ConstructedTypeDecl->decls() ) {
+        if( const TypedefNameDecl *ND = dyn_cast<TypedefNameDecl>( D ) ) {
+            if( ND->getName() == "result_type" ) {
+                FoundResultType = true;
+                break;
+            }
+        }
+    }
+    if( !FoundResultType ) {
+        return;
+    }
+    const QualType ConstructedType = Construction->getType();
+    Check.diag(
+        Construction->getBeginLoc(),
+        "Construction of library random engine %0.  To ensure determinism for a fixed seed, "
+        "use the common tools from rng.h rather than your own random number engines." )
+            << ConstructedType;
+}
+
+void DeterminismCheck::check( const MatchFinder::MatchResult &Result )
+{
+    CheckCall( *this, Result );
+    CheckInit( *this, Result );
+}
+
+} // namespace cata
+} // namespace tidy
+} // namespace clang

tools/clang-tidy-plugin/DeterminismCheck.h

@@ -0,0 +1,32 @@
+#ifndef CATA_TOOLS_CLANG_TIDY_PLUGIN_DETERMINISMCHECK_H
+#define CATA_TOOLS_CLANG_TIDY_PLUGIN_DETERMINISMCHECK_H
+
+#include <clang/ASTMatchers/ASTMatchFinder.h>
+#include <llvm/ADT/StringRef.h>
+
+#include "ClangTidy.h"
+
+namespace clang
+{
+
+namespace tidy
+{
+class ClangTidyContext;
+
+namespace cata
+{
+
+class DeterminismCheck : public ClangTidyCheck
+{
+    public:
+        DeterminismCheck( StringRef Name, ClangTidyContext *Context )
+            : ClangTidyCheck( Name, Context ) {}
+        void registerMatchers( ast_matchers::MatchFinder *Finder ) override;
+        void check( const ast_matchers::MatchFinder::MatchResult &Result ) override;
+};
+
+} // namespace cata
+} // namespace tidy
+} // namespace clang
+
+#endif // CATA_TOOLS_CLANG_TIDY_PLUGIN_DETERMINISMCHECK_H

tools/clang-tidy-plugin/test/determinism.cpp

@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s cata-determinism %t -- -plugins=%cata_plugin --
+
+using size_t = unsigned long;
+
+namespace std
+{
+
+using uint_fast32_t = unsigned;
+
+template <
+
+    class UIntType,
+    UIntType a,
+    UIntType c,
+    UIntType m
+    > struct linear_congruential_engine
+{
+    using result_type = UIntType;
+    explicit linear_congruential_engine( result_type value );
+    void seed( result_type value );
+};
+using minstd_rand0 = std::linear_congruential_engine<std::uint_fast32_t, 16807, 0, 2147483647>;
+
+template <
+
+    class UIntType,
+    size_t w, size_t n, size_t m, size_t r,
+    UIntType a, size_t u, UIntType d, size_t s,
+    UIntType b, size_t t,
+    UIntType c, size_t l, UIntType f
+    > struct mersenne_twister_engine
+{
+    using result_type = UIntType;
+    void seed( result_type value );
+};
+using mt19937 = std::mersenne_twister_engine<std::uint_fast32_t, 32, 624, 397, 31,
+      0x9908b0df, 11,
+      0xffffffff, 7,
+      0x9d2c5680, 15,
+      0xefc60000, 18, 1812433253>;
+}
+
+struct SomeOtherStruct {
+    int i;
+    void foo();
+};
+
+struct StructWithSeedButNotResultType {
+    void seed();
+};
+
+using SomeOtherAlias = SomeOtherStruct;
+
+int rand();
+
+using cata_default_random_engine = std::minstd_rand0;
+
+namespace std
+{
+using ::rand;
+}
+
+namespace other
+{
+int rand();
+}
+
+void f()
+{
+    std::mt19937 gen0;
+    // CHECK-MESSAGES: [[@LINE-1]]:18: warning: Construction of library random engine 'std::mt19937' (aka 'mersenne_twister_engine<unsigned int, 32, 624, 397, 31, 2567483615U, 11, 4294967295U, 7, 2636928640U, 15, 4022730752U, 18, 1812433253>').  To ensure determinism for a fixed seed, use the common tools from rng.h rather than your own random number engines. [cata-determinism]
+    cata_default_random_engine gen1( 0 );
+    // CHECK-MESSAGES: [[@LINE-1]]:32: warning: Construction of library random engine 'cata_default_random_engine' (aka 'linear_congruential_engine<unsigned int, 16807, 0, 2147483647>').  To ensure determinism for a fixed seed, use the common tools from rng.h rather than your own random number engines. [cata-determinism]
+    SomeOtherStruct s0;
+    StructWithSeedButNotResultType s1;
+    SomeOtherAlias a;
+    rand();
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: Call to library random function 'rand'.  To ensure determinism for a fixed seed, use the common tools from rng.h rather than calling library random functions. [cata-determinism]
+    std::rand();
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: Call to library random function 'rand'.  To ensure determinism for a fixed seed, use the common tools from rng.h rather than calling library random functions. [cata-determinism]
+    other::rand();
+}