[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho added a comment. Thanks ! https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman closed this revision. aaron.ballman added a comment. Thanks! I've commit in r285809 https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho added a comment. I just figured out that I don't have right to commit to llvm so I would appreciate if you could commit this check for me. Do you need any info about me? Thank you! https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho added a comment. Thanks but I think I will try it! https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added a comment. In https://reviews.llvm.org/D22346#581329, @falho wrote: > Cool! Thank you for the reviews! If you don't have commit privileges, let me know and I'm happy to commit on your behalf. https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho added a comment. Cool! Thank you for the reviews! https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for working on this! https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho updated this revision to Diff 76072.
falho marked an inline comment as done.
falho added a comment.
in cpp diagnostics message: comma changed back to semicolon, + curly braces
removed
testfiles corrected accordingly
https://reviews.llvm.org/D22346
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/cert/LimitedRandomnessCheck.cpp
clang-tidy/cert/LimitedRandomnessCheck.h
docs/clang-tidy/checks/cert-msc30-c.rst
docs/clang-tidy/checks/cert-msc50-cpp.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cert-limited-randomness.c
test/clang-tidy/cert-limited-randomness.cpp
Index: test/clang-tidy/cert-limited-randomness.cpp
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cert-msc50-cpp %t
+
+int rand();
+int rand(int);
+
+namespace std {
+using ::rand;
+}
+
+namespace nonstd {
+ int rand();
+}
+
+void testFunction1() {
+ int i = std::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness; use C++11 random library instead [cert-msc50-cpp]
+
+ int j = ::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness; use C++11 random library instead [cert-msc50-cpp]
+
+ int k = rand(i);
+
+ int l = nonstd::rand();
+
+ int m = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness; use C++11 random library instead [cert-msc50-cpp]
+}
+
Index: test/clang-tidy/cert-limited-randomness.c
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.c
@@ -0,0 +1,13 @@
+// RUN: %check_clang_tidy %s cert-msc30-c %t
+
+extern int rand(void);
+int nonrand();
+
+int cTest() {
+ int i = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness [cert-msc30-c]
+
+ int k = nonrand();
+
+ return 0;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -18,6 +18,8 @@
cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference)
cert-fio38-c (redirects to misc-non-copyable-objects)
cert-flp30-c
+ cert-msc30-c (redirects to cert-limited-randomness)
+ cert-msc50-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init)
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cert-msc50-cpp.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc50-cpp.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - cert-msc50-cpp
+
+cert-msc50-cpp
+==
+
+Pseudorandom number generators use mathematical algorithms to produce a sequence of numbers with good statistical properties, but the numbers produced are not genuinely random. The ``std::rand()`` function takes a seed (number), runs a mathematical operation on it and returns the result. By manipulating the seed the result can be predictible. This check warns for the usage of ``std::rand()``.
Index: docs/clang-tidy/checks/cert-msc30-c.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc30-c.rst
@@ -0,0 +1,7 @@
+.. title:: clang-tidy - cert-msc30-c
+
+cert-msc30-c
+
+
+The cert-msc30-c check is an alias, please see
+`cert-msc50-cpp `_ for more information.
Index: clang-tidy/cert/LimitedRandomnessCheck.h
===
--- /dev/null
+++ clang-tidy/cert/LimitedRandomnessCheck.h
@@ -0,0 +1,38 @@
+//===--- LimitedRandomnessCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Pseudorandom number generators are not genuinely random. The result of the
+/// std::rand() function makes no guarantees as to the quality of the random
+/// sequence produced.
+/// This check warns for the usage of std::rand() function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc50-cpp.html
+class LimitedRandomnessCheck : public ClangTidyCheck {
+public:
+ LimitedRandomnessCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFi
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added a comment.
In https://reviews.llvm.org/D22346#580242, @falho wrote:
> removed semicolon, and replaced it with a comma that only appears in .cpp
> diagnostics
The semicolon was the correct punctuator to use, but thank you for moving it
into the cpp message.
> test cases corrected according to this
>
> removed junk .swo file
Thanks!
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:30
+ if (getLangOpts().CPlusPlus) {
+msg = ", use C++11 random library instead";
+ }
Switch the "," to ";", and remove the curly braces.
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho updated this revision to Diff 75935.
falho marked an inline comment as done.
falho added a comment.
removed semicolon, and replaced it with a comma that only appears in .cpp
diagnostics
test cases corrected according to this
removed junk .swo file
https://reviews.llvm.org/D22346
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/cert/LimitedRandomnessCheck.cpp
clang-tidy/cert/LimitedRandomnessCheck.h
docs/clang-tidy/checks/cert-msc30-c.rst
docs/clang-tidy/checks/cert-msc50-cpp.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cert-limited-randomness.c
test/clang-tidy/cert-limited-randomness.cpp
Index: test/clang-tidy/cert-limited-randomness.cpp
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cert-msc50-cpp %t
+
+int rand();
+int rand(int);
+
+namespace std {
+using ::rand;
+}
+
+namespace nonstd {
+ int rand();
+}
+
+void testFunction1() {
+ int i = std::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+ int j = ::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+ int k = rand(i);
+
+ int l = nonstd::rand();
+
+ int m = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+}
+
Index: test/clang-tidy/cert-limited-randomness.c
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.c
@@ -0,0 +1,13 @@
+// RUN: %check_clang_tidy %s cert-msc30-c %t
+
+extern int rand(void);
+int nonrand();
+
+int cTest() {
+ int i = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness [cert-msc30-c]
+
+ int k = nonrand();
+
+ return 0;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -18,6 +18,8 @@
cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference)
cert-fio38-c (redirects to misc-non-copyable-objects)
cert-flp30-c
+ cert-msc30-c (redirects to cert-limited-randomness)
+ cert-msc50-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init)
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cert-msc50-cpp.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc50-cpp.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - cert-msc50-cpp
+
+cert-msc50-cpp
+==
+
+Pseudorandom number generators use mathematical algorithms to produce a sequence of numbers with good statistical properties, but the numbers produced are not genuinely random. The ``std::rand()`` function takes a seed (number), runs a mathematical operation on it and returns the result. By manipulating the seed the result can be predictible. This check warns for the usage of ``std::rand()``.
Index: docs/clang-tidy/checks/cert-msc30-c.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc30-c.rst
@@ -0,0 +1,7 @@
+.. title:: clang-tidy - cert-msc30-c
+
+cert-msc30-c
+
+
+The cert-msc30-c check is an alias, please see
+`cert-msc50-cpp `_ for more information.
Index: clang-tidy/cert/LimitedRandomnessCheck.h
===
--- /dev/null
+++ clang-tidy/cert/LimitedRandomnessCheck.h
@@ -0,0 +1,38 @@
+//===--- LimitedRandomnessCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Pseudorandom number generators are not genuinely random. The result of the
+/// std::rand() function makes no guarantees as to the quality of the random
+/// sequence produced.
+/// This check warns for the usage of std::rand() function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc50-cpp.html
+class LimitedRandomnessCheck : public ClangTidyCheck {
+public:
+ LimitedRandomnessCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added a comment. clang-tidy/cert/.LimitedRandomnessCheck.cpp.swo was added and should not have been; also, there's one minor issue with the diagnostic wording that is still outstanding. Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:35 + diag(MatchedDecl->getLocStart(), + "rand() function has limited randomness; " + msg); +} aaron.ballman wrote: > For C code, this diagnostic will read strangely due to the trailing > semicolon. You should move the semicolon into the `msg` above. Perhaps we can > also drop "function" from the diagnostic as well. This comment does not appear to have been handled -- the semicolon is still present even when `msg` is empty. https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho updated this revision to Diff 75460.
falho added a comment.
changes according to 2nd review
https://reviews.llvm.org/D22346
Files:
clang-tidy/cert/.LimitedRandomnessCheck.cpp.swo
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/cert/LimitedRandomnessCheck.cpp
clang-tidy/cert/LimitedRandomnessCheck.h
docs/clang-tidy/checks/cert-msc30-c.rst
docs/clang-tidy/checks/cert-msc50-cpp.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cert-limited-randomness.c
test/clang-tidy/cert-limited-randomness.cpp
Index: test/clang-tidy/cert-limited-randomness.cpp
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cert-msc50-cpp %t
+
+int rand();
+int rand(int);
+
+namespace std {
+using ::rand;
+}
+
+namespace nonstd {
+ int rand();
+}
+
+void testFunction1() {
+ int i = std::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+ int j = ::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+ int k = rand(i);
+
+ int l = nonstd::rand();
+
+ int m = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+}
+
Index: test/clang-tidy/cert-limited-randomness.c
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.c
@@ -0,0 +1,13 @@
+// RUN: %check_clang_tidy %s cert-msc30-c %t
+
+extern int rand(void);
+int nonrand();
+
+int cTest() {
+ int i = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness; [cert-msc30-c]
+
+ int k = nonrand();
+
+ return 0;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -18,6 +18,8 @@
cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference)
cert-fio38-c (redirects to misc-non-copyable-objects)
cert-flp30-c
+ cert-msc30-c (redirects to cert-limited-randomness)
+ cert-msc50-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init)
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cert-msc50-cpp.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc50-cpp.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - cert-msc50-cpp
+
+cert-msc50-cpp
+==
+
+Pseudorandom number generators use mathematical algorithms to produce a sequence of numbers with good statistical properties, but the numbers produced are not genuinely random. The ``std::rand()`` function takes a seed (number), runs a mathematical operation on it and returns the result. By manipulating the seed the result can be predictible. This check warns for the usage of ``std::rand()``.
Index: docs/clang-tidy/checks/cert-msc30-c.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc30-c.rst
@@ -0,0 +1,7 @@
+.. title:: clang-tidy - cert-msc30-c
+
+cert-msc30-c
+
+
+The cert-msc30-c check is an alias, please see
+`cert-msc50-cpp `_ for more information.
Index: clang-tidy/cert/LimitedRandomnessCheck.h
===
--- /dev/null
+++ clang-tidy/cert/LimitedRandomnessCheck.h
@@ -0,0 +1,38 @@
+//===--- LimitedRandomnessCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Pseudorandom number generators are not genuinely random. The result of the
+/// std::rand() function makes no guarantees as to the quality of the random
+/// sequence produced.
+/// This check warns for the usage of std::rand() function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc50-cpp.html
+class LimitedRandomnessCheck : public ClangTidyCheck {
+public:
+ LimitedRandomnessCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // name
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added a comment. I noticed you marked several comments as done, but the patch is not updated with changes. Have I missed something? https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added a comment. Thank you for continuing your efforts on this, I have just a few minor nits remaining. Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:35 + diag(MatchedDecl->getLocStart(), + "rand() function has limited randomness; " + msg); +} For C code, this diagnostic will read strangely due to the trailing semicolon. You should move the semicolon into the `msg` above. Perhaps we can also drop "function" from the diagnostic as well. Comment at: docs/clang-tidy/checks/cert-msc50-cpp.rst:3 + +cert-msc-50 +=== This should be cert-msc50-cpp instead. Comment at: docs/clang-tidy/checks/list.rst:20 cert-flp30-c + cert-msc50-cpp cert-oop11-cpp (redirects to misc-move-constructor-init) aaron.ballman wrote: > Please also add a cert-msc30-c file with a redirect (like fio38-c from above). This should be cert-msc30-c https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho removed rL LLVM as the repository for this revision.
falho updated this revision to Diff 74908.
falho added a comment.
Herald added subscribers: modocache, mgorny, beanz.
updated diff according to first reviews
https://reviews.llvm.org/D22346
Files:
clang-tidy/cert/.LimitedRandomnessCheck.cpp.swo
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/cert/LimitedRandomnessCheck.cpp
clang-tidy/cert/LimitedRandomnessCheck.h
docs/clang-tidy/checks/cert-msc50-cpp.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cert-limited-randomness.c
test/clang-tidy/cert-limited-randomness.cpp
Index: test/clang-tidy/cert-limited-randomness.cpp
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cert-msc50-cpp %t
+
+int rand();
+int rand(int);
+
+namespace std {
+using ::rand;
+}
+
+namespace nonstd {
+ int rand();
+}
+
+void testFunction1() {
+ int i = std::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: rand() function has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+ int j = ::rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: rand() function has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+ int k = rand(i);
+
+ int l = nonstd::rand();
+
+ int m = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() function has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+}
+
Index: test/clang-tidy/cert-limited-randomness.c
===
--- /dev/null
+++ test/clang-tidy/cert-limited-randomness.c
@@ -0,0 +1,13 @@
+// RUN: %check_clang_tidy %s cert-msc30-c %t
+
+extern int rand(void);
+int nonrand();
+
+int cTest() {
+ int i = rand();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() function has limited randomness; [cert-msc30-c]
+
+ int k = nonrand();
+
+ return 0;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -17,6 +17,8 @@
cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference)
cert-fio38-c (redirects to misc-non-copyable-objects)
cert-flp30-c
+ cert-msc50-c (redirects to cert-limited-randomness)
+ cert-msc50-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init)
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cert-msc50-cpp.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc50-cpp.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - cert-msc50-cpp
+
+cert-msc-50
+===
+
+Pseudorandom number generators use mathematical algorithms to produce a sequence of numbers with good statistical properties, but the numbers produced are not genuinely random. The ``std::rand()`` function takes a seed (number), runs a mathematical operation on it and returns the result. By manipulating the seed the result can be predictible. This check warns for the usage of ``std::rand()``.
Index: clang-tidy/cert/LimitedRandomnessCheck.h
===
--- /dev/null
+++ clang-tidy/cert/LimitedRandomnessCheck.h
@@ -0,0 +1,38 @@
+//===--- LimitedRandomnessCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Pseudorandom number generators are not genuinely random. The result of the
+/// std::rand() function makes no guarantees as to the quality of the random
+/// sequence produced.
+/// This check warns for the usage of std::rand() function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc50-cpp.html
+class LimitedRandomnessCheck : public ClangTidyCheck {
+public:
+ LimitedRandomnessCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
Index: clang-tidy/cert/LimitedRandomnessCheck.cpp
===
--- /dev/null
+++
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
falho wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > xazax.hun wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Prazek wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Prazek wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > This should be looking at a callExpr() rather than a
> > > > > > > > > > > declRefExpr(), should it not?
> > > > > > > > > > I was also thinking about this, but this is actually
> > > > > > > > > > better, because it will also match with binding rand with
> > > > > > > > > > function pointer.
> > > > > > > > > True, but a DeclRefExpr doesn't mean it's a function call.
> > > > > > > > > Binding the function is not contrary to the CERT rule, just
> > > > > > > > > calling it. For instance, the following pathological case
> > > > > > > > > will be caught by this check:
> > > > > > > > > ```
> > > > > > > > > if (std::rand) {}
> > > > > > > > > ```
> > > > > > > > > The behavior of this check should be consistent with
> > > > > > > > > cert-env33-c, which only looks at calls. (If we really care
> > > > > > > > > about bound functions, we'd need flow control analysis, and I
> > > > > > > > > think that's overkill for both of those checks, but wouldn't
> > > > > > > > > be opposed to someone writing the flow analysis if they
> > > > > > > > > really wanted to.)
> > > > > > > > It would indeed fire on this pathological case, but I don't
> > > > > > > > think we should care about cases like this, because no one is
> > > > > > > > writing code like this (and if he would then it would probably
> > > > > > > > be a bug).
> > > > > > > > I don't think that there is much code that binds pointer to
> > > > > > > > std::rand either, but I think it would be good to display
> > > > > > > > warning for this, because even if the function would be never
> > > > > > > > called, then it means that this is a bug, and if it would be
> > > > > > > > called then it would be nice to tell user that rand might be
> > > > > > > > used here.
> > > > > > > >
> > > > > > > > Anyway I don't oppose for changing it to callExpr, but I think
> > > > > > > > it is better this way.
> > > > > > > > It would indeed fire on this pathological case, but I don't
> > > > > > > > think we should care about cases like this, because no one is
> > > > > > > > writing code like this (and if he would then it would probably
> > > > > > > > be a bug).
> > > > > > >
> > > > > > > It would be a known false-positive for a check designed to
> > > > > > > conform to a particular coding standard. When deviations have
> > > > > > > come up in the past for various coding standards, we've added an
> > > > > > > option to enable the additional functionality, which I don't
> > > > > > > think would be reasonable in this case. Alternatively, the CERT
> > > > > > > guideline could be modified, but that is unlikely to occur
> > > > > > > because binding the function pointer is not a security concern
> > > > > > > (only calling the function).
> > > > > > In case you let binding to function pointer you introduce potential
> > > > > > false negatives which is worse in this case in my opinion.
> > > > > Basically: this half-measure is sufficient for the CERT coding rule,
> > > > > but isn't ideal. The ideal check isn't likely to uncover many more
> > > > > cases than the half-measure, which is why it was not implemented in
> > > > > the past. If someone wants to implement the whole-measure, that's
> > > > > great! But implementing a half, half-measure that isn't consistent
> > > > > with other, similar checks is the wrong thing to do.
> > > > You can not implement an ideal checker. In general, it is undecidable
> > > > whether std::rand is called or not. (You can easily create an example
> > > > where you would need to solve the halting problem in order to decide
> > > > whether std::rand is called.)
> > > >
> > > > Since the ideal checker is infeasible the question is whether you are
> > > > OK with false positives or false negatives. The approach you are
> > > > suggesting result in false negatives. The current approach results in
> > > > false positives. I think, for this security checker, a false positive
> > > > is much less serious to have than a false negative. Moreover, I doubt
> > > > that people write code where such false positives are intended and the
> > > > code should not be changed. But in case you can think of an example,
> > > > please let us know.
> > > > You can not implement an ideal checker. In general, it is undecidable
> > > > whether s
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho marked 5 inline comments as done.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > xazax.hun wrote:
> > > > > aaron.ballman wrote:
> > > > > > Prazek wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Prazek wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > This should be looking at a callExpr() rather than a
> > > > > > > > > > declRefExpr(), should it not?
> > > > > > > > > I was also thinking about this, but this is actually better,
> > > > > > > > > because it will also match with binding rand with function
> > > > > > > > > pointer.
> > > > > > > > True, but a DeclRefExpr doesn't mean it's a function call.
> > > > > > > > Binding the function is not contrary to the CERT rule, just
> > > > > > > > calling it. For instance, the following pathological case will
> > > > > > > > be caught by this check:
> > > > > > > > ```
> > > > > > > > if (std::rand) {}
> > > > > > > > ```
> > > > > > > > The behavior of this check should be consistent with
> > > > > > > > cert-env33-c, which only looks at calls. (If we really care
> > > > > > > > about bound functions, we'd need flow control analysis, and I
> > > > > > > > think that's overkill for both of those checks, but wouldn't be
> > > > > > > > opposed to someone writing the flow analysis if they really
> > > > > > > > wanted to.)
> > > > > > > It would indeed fire on this pathological case, but I don't think
> > > > > > > we should care about cases like this, because no one is writing
> > > > > > > code like this (and if he would then it would probably be a bug).
> > > > > > > I don't think that there is much code that binds pointer to
> > > > > > > std::rand either, but I think it would be good to display warning
> > > > > > > for this, because even if the function would be never called,
> > > > > > > then it means that this is a bug, and if it would be called then
> > > > > > > it would be nice to tell user that rand might be used here.
> > > > > > >
> > > > > > > Anyway I don't oppose for changing it to callExpr, but I think it
> > > > > > > is better this way.
> > > > > > > It would indeed fire on this pathological case, but I don't think
> > > > > > > we should care about cases like this, because no one is writing
> > > > > > > code like this (and if he would then it would probably be a bug).
> > > > > >
> > > > > > It would be a known false-positive for a check designed to conform
> > > > > > to a particular coding standard. When deviations have come up in
> > > > > > the past for various coding standards, we've added an option to
> > > > > > enable the additional functionality, which I don't think would be
> > > > > > reasonable in this case. Alternatively, the CERT guideline could be
> > > > > > modified, but that is unlikely to occur because binding the
> > > > > > function pointer is not a security concern (only calling the
> > > > > > function).
> > > > > In case you let binding to function pointer you introduce potential
> > > > > false negatives which is worse in this case in my opinion.
> > > > Basically: this half-measure is sufficient for the CERT coding rule,
> > > > but isn't ideal. The ideal check isn't likely to uncover many more
> > > > cases than the half-measure, which is why it was not implemented in the
> > > > past. If someone wants to implement the whole-measure, that's great!
> > > > But implementing a half, half-measure that isn't consistent with other,
> > > > similar checks is the wrong thing to do.
> > > You can not implement an ideal checker. In general, it is undecidable
> > > whether std::rand is called or not. (You can easily create an example
> > > where you would need to solve the halting problem in order to decide
> > > whether std::rand is called.)
> > >
> > > Since the ideal checker is infeasible the question is whether you are OK
> > > with false positives or false negatives. The approach you are suggesting
> > > result in false negatives. The current approach results in false
> > > positives. I think, for this security checker, a false positive is much
> > > less serious to have than a false negative. Moreover, I doubt that people
> > > write code where such false positives are intended and the code should
> > > not be changed. But in case you can think of an example, please let us
> > > know.
> > > You can not implement an ideal checker. In general, it is undecidable
> > > whether std::rand is called or not. (You can easily create an example
> > > where you would need to solve the halting problem in order to decide
> > > whether std::rand is called.)
> >
> > I said "ideal", not "perfe
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > Prazek wrote:
> > > > > aaron.ballman wrote:
> > > > > > Prazek wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > This should be looking at a callExpr() rather than a
> > > > > > > > declRefExpr(), should it not?
> > > > > > > I was also thinking about this, but this is actually better,
> > > > > > > because it will also match with binding rand with function
> > > > > > > pointer.
> > > > > > True, but a DeclRefExpr doesn't mean it's a function call. Binding
> > > > > > the function is not contrary to the CERT rule, just calling it. For
> > > > > > instance, the following pathological case will be caught by this
> > > > > > check:
> > > > > > ```
> > > > > > if (std::rand) {}
> > > > > > ```
> > > > > > The behavior of this check should be consistent with cert-env33-c,
> > > > > > which only looks at calls. (If we really care about bound
> > > > > > functions, we'd need flow control analysis, and I think that's
> > > > > > overkill for both of those checks, but wouldn't be opposed to
> > > > > > someone writing the flow analysis if they really wanted to.)
> > > > > It would indeed fire on this pathological case, but I don't think we
> > > > > should care about cases like this, because no one is writing code
> > > > > like this (and if he would then it would probably be a bug).
> > > > > I don't think that there is much code that binds pointer to std::rand
> > > > > either, but I think it would be good to display warning for this,
> > > > > because even if the function would be never called, then it means
> > > > > that this is a bug, and if it would be called then it would be nice
> > > > > to tell user that rand might be used here.
> > > > >
> > > > > Anyway I don't oppose for changing it to callExpr, but I think it is
> > > > > better this way.
> > > > > It would indeed fire on this pathological case, but I don't think we
> > > > > should care about cases like this, because no one is writing code
> > > > > like this (and if he would then it would probably be a bug).
> > > >
> > > > It would be a known false-positive for a check designed to conform to a
> > > > particular coding standard. When deviations have come up in the past
> > > > for various coding standards, we've added an option to enable the
> > > > additional functionality, which I don't think would be reasonable in
> > > > this case. Alternatively, the CERT guideline could be modified, but
> > > > that is unlikely to occur because binding the function pointer is not a
> > > > security concern (only calling the function).
> > > In case you let binding to function pointer you introduce potential false
> > > negatives which is worse in this case in my opinion.
> > Basically: this half-measure is sufficient for the CERT coding rule, but
> > isn't ideal. The ideal check isn't likely to uncover many more cases than
> > the half-measure, which is why it was not implemented in the past. If
> > someone wants to implement the whole-measure, that's great! But
> > implementing a half, half-measure that isn't consistent with other, similar
> > checks is the wrong thing to do.
> You can not implement an ideal checker. In general, it is undecidable whether
> std::rand is called or not. (You can easily create an example where you would
> need to solve the halting problem in order to decide whether std::rand is
> called.)
>
> Since the ideal checker is infeasible the question is whether you are OK with
> false positives or false negatives. The approach you are suggesting result in
> false negatives. The current approach results in false positives. I think,
> for this security checker, a false positive is much less serious to have than
> a false negative. Moreover, I doubt that people write code where such false
> positives are intended and the code should not be changed. But in case you
> can think of an example, please let us know.
> You can not implement an ideal checker. In general, it is undecidable whether
> std::rand is called or not. (You can easily create an example where you would
> need to solve the halting problem in order to decide whether std::rand is
> called.)
I said "ideal", not "perfect", but we're splitting hairs. You are correct,
you're never going to get perfect clarity without runtime instrumentation. By
"ideal", I meant "something that actually pays attention to the bound value
from the point it is bound to the point the function pointer is called." Simply
having the address of the function is not a security concern; calling it is.
> I thi
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
xazax.hun added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
xazax.hun wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > Prazek wrote:
> > > > > aaron.ballman wrote:
> > > > > > Prazek wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > This should be looking at a callExpr() rather than a
> > > > > > > > declRefExpr(), should it not?
> > > > > > > I was also thinking about this, but this is actually better,
> > > > > > > because it will also match with binding rand with function
> > > > > > > pointer.
> > > > > > True, but a DeclRefExpr doesn't mean it's a function call. Binding
> > > > > > the function is not contrary to the CERT rule, just calling it. For
> > > > > > instance, the following pathological case will be caught by this
> > > > > > check:
> > > > > > ```
> > > > > > if (std::rand) {}
> > > > > > ```
> > > > > > The behavior of this check should be consistent with cert-env33-c,
> > > > > > which only looks at calls. (If we really care about bound
> > > > > > functions, we'd need flow control analysis, and I think that's
> > > > > > overkill for both of those checks, but wouldn't be opposed to
> > > > > > someone writing the flow analysis if they really wanted to.)
> > > > > It would indeed fire on this pathological case, but I don't think we
> > > > > should care about cases like this, because no one is writing code
> > > > > like this (and if he would then it would probably be a bug).
> > > > > I don't think that there is much code that binds pointer to std::rand
> > > > > either, but I think it would be good to display warning for this,
> > > > > because even if the function would be never called, then it means
> > > > > that this is a bug, and if it would be called then it would be nice
> > > > > to tell user that rand might be used here.
> > > > >
> > > > > Anyway I don't oppose for changing it to callExpr, but I think it is
> > > > > better this way.
> > > > > It would indeed fire on this pathological case, but I don't think we
> > > > > should care about cases like this, because no one is writing code
> > > > > like this (and if he would then it would probably be a bug).
> > > >
> > > > It would be a known false-positive for a check designed to conform to a
> > > > particular coding standard. When deviations have come up in the past
> > > > for various coding standards, we've added an option to enable the
> > > > additional functionality, which I don't think would be reasonable in
> > > > this case. Alternatively, the CERT guideline could be modified, but
> > > > that is unlikely to occur because binding the function pointer is not a
> > > > security concern (only calling the function).
> > > In case you let binding to function pointer you introduce potential false
> > > negatives which is worse in this case in my opinion.
> > Basically: this half-measure is sufficient for the CERT coding rule, but
> > isn't ideal. The ideal check isn't likely to uncover many more cases than
> > the half-measure, which is why it was not implemented in the past. If
> > someone wants to implement the whole-measure, that's great! But
> > implementing a half, half-measure that isn't consistent with other, similar
> > checks is the wrong thing to do.
> You can not implement an ideal checker. In general, it is undecidable whether
> std::rand is called or not. (You can easily create an example where you would
> need to solve the halting problem in order to decide whether std::rand is
> called.)
>
> Since the ideal checker is infeasible the question is whether you are OK with
> false positives or false negatives. The approach you are suggesting result in
> false negatives. The current approach results in false positives. I think,
> for this security checker, a false positive is much less serious to have than
> a false negative. Moreover, I doubt that people write code where such false
> positives are intended and the code should not be changed. But in case you
> can think of an example, please let us know.
I think consisteny with other checks are not always a good argument. You might
want to ask what is the expected false positive and false nagtive rate from a
check, and what is the guarantee that a user expects from a check. And I think
base on that it is a unique decision that should be considered independently
for each check. In this case I think it is more valuable to have a guarantee
that in case all of the code is covered, std::rand() is not invoked. Using a
callExpr instead of declRefExpr we would loose this guarantee at the cost of
not reporting some false positive cases that are unlikely to annoy anyone
anyways.
Repository:
rL
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
xazax.hun added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > Prazek wrote:
> > > > aaron.ballman wrote:
> > > > > Prazek wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > This should be looking at a callExpr() rather than a
> > > > > > > declRefExpr(), should it not?
> > > > > > I was also thinking about this, but this is actually better,
> > > > > > because it will also match with binding rand with function pointer.
> > > > > True, but a DeclRefExpr doesn't mean it's a function call. Binding
> > > > > the function is not contrary to the CERT rule, just calling it. For
> > > > > instance, the following pathological case will be caught by this
> > > > > check:
> > > > > ```
> > > > > if (std::rand) {}
> > > > > ```
> > > > > The behavior of this check should be consistent with cert-env33-c,
> > > > > which only looks at calls. (If we really care about bound functions,
> > > > > we'd need flow control analysis, and I think that's overkill for both
> > > > > of those checks, but wouldn't be opposed to someone writing the flow
> > > > > analysis if they really wanted to.)
> > > > It would indeed fire on this pathological case, but I don't think we
> > > > should care about cases like this, because no one is writing code like
> > > > this (and if he would then it would probably be a bug).
> > > > I don't think that there is much code that binds pointer to std::rand
> > > > either, but I think it would be good to display warning for this,
> > > > because even if the function would be never called, then it means that
> > > > this is a bug, and if it would be called then it would be nice to tell
> > > > user that rand might be used here.
> > > >
> > > > Anyway I don't oppose for changing it to callExpr, but I think it is
> > > > better this way.
> > > > It would indeed fire on this pathological case, but I don't think we
> > > > should care about cases like this, because no one is writing code like
> > > > this (and if he would then it would probably be a bug).
> > >
> > > It would be a known false-positive for a check designed to conform to a
> > > particular coding standard. When deviations have come up in the past for
> > > various coding standards, we've added an option to enable the additional
> > > functionality, which I don't think would be reasonable in this case.
> > > Alternatively, the CERT guideline could be modified, but that is unlikely
> > > to occur because binding the function pointer is not a security concern
> > > (only calling the function).
> > In case you let binding to function pointer you introduce potential false
> > negatives which is worse in this case in my opinion.
> Basically: this half-measure is sufficient for the CERT coding rule, but
> isn't ideal. The ideal check isn't likely to uncover many more cases than the
> half-measure, which is why it was not implemented in the past. If someone
> wants to implement the whole-measure, that's great! But implementing a half,
> half-measure that isn't consistent with other, similar checks is the wrong
> thing to do.
You can not implement an ideal checker. In general, it is undecidable whether
std::rand is called or not. (You can easily create an example where you would
need to solve the halting problem in order to decide whether std::rand is
called.)
Since the ideal checker is infeasible the question is whether you are OK with
false positives or false negatives. The approach you are suggesting result in
false negatives. The current approach results in false positives. I think, for
this security checker, a false positive is much less serious to have than a
false negative. Moreover, I doubt that people write code where such false
positives are intended and the code should not be changed. But in case you can
think of an example, please let us know.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
xazax.hun wrote:
> aaron.ballman wrote:
> > Prazek wrote:
> > > aaron.ballman wrote:
> > > > Prazek wrote:
> > > > > aaron.ballman wrote:
> > > > > > This should be looking at a callExpr() rather than a declRefExpr(),
> > > > > > should it not?
> > > > > I was also thinking about this, but this is actually better, because
> > > > > it will also match with binding rand with function pointer.
> > > > True, but a DeclRefExpr doesn't mean it's a function call. Binding the
> > > > function is not contrary to the CERT rule, just calling it. For
> > > > instance, the following pathological case will be caught by this check:
> > > > ```
> > > > if (std::rand) {}
> > > > ```
> > > > The behavior of this check should be consistent with cert-env33-c,
> > > > which only looks at calls. (If we really care about bound functions,
> > > > we'd need flow control analysis, and I think that's overkill for both
> > > > of those checks, but wouldn't be opposed to someone writing the flow
> > > > analysis if they really wanted to.)
> > > It would indeed fire on this pathological case, but I don't think we
> > > should care about cases like this, because no one is writing code like
> > > this (and if he would then it would probably be a bug).
> > > I don't think that there is much code that binds pointer to std::rand
> > > either, but I think it would be good to display warning for this, because
> > > even if the function would be never called, then it means that this is a
> > > bug, and if it would be called then it would be nice to tell user that
> > > rand might be used here.
> > >
> > > Anyway I don't oppose for changing it to callExpr, but I think it is
> > > better this way.
> > > It would indeed fire on this pathological case, but I don't think we
> > > should care about cases like this, because no one is writing code like
> > > this (and if he would then it would probably be a bug).
> >
> > It would be a known false-positive for a check designed to conform to a
> > particular coding standard. When deviations have come up in the past for
> > various coding standards, we've added an option to enable the additional
> > functionality, which I don't think would be reasonable in this case.
> > Alternatively, the CERT guideline could be modified, but that is unlikely
> > to occur because binding the function pointer is not a security concern
> > (only calling the function).
> In case you let binding to function pointer you introduce potential false
> negatives which is worse in this case in my opinion.
Basically: this half-measure is sufficient for the CERT coding rule, but isn't
ideal. The ideal check isn't likely to uncover many more cases than the
half-measure, which is why it was not implemented in the past. If someone wants
to implement the whole-measure, that's great! But implementing a half,
half-measure that isn't consistent with other, similar checks is the wrong
thing to do.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
xazax.hun added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
aaron.ballman wrote:
> Prazek wrote:
> > aaron.ballman wrote:
> > > Prazek wrote:
> > > > aaron.ballman wrote:
> > > > > This should be looking at a callExpr() rather than a declRefExpr(),
> > > > > should it not?
> > > > I was also thinking about this, but this is actually better, because it
> > > > will also match with binding rand with function pointer.
> > > True, but a DeclRefExpr doesn't mean it's a function call. Binding the
> > > function is not contrary to the CERT rule, just calling it. For instance,
> > > the following pathological case will be caught by this check:
> > > ```
> > > if (std::rand) {}
> > > ```
> > > The behavior of this check should be consistent with cert-env33-c, which
> > > only looks at calls. (If we really care about bound functions, we'd need
> > > flow control analysis, and I think that's overkill for both of those
> > > checks, but wouldn't be opposed to someone writing the flow analysis if
> > > they really wanted to.)
> > It would indeed fire on this pathological case, but I don't think we should
> > care about cases like this, because no one is writing code like this (and
> > if he would then it would probably be a bug).
> > I don't think that there is much code that binds pointer to std::rand
> > either, but I think it would be good to display warning for this, because
> > even if the function would be never called, then it means that this is a
> > bug, and if it would be called then it would be nice to tell user that rand
> > might be used here.
> >
> > Anyway I don't oppose for changing it to callExpr, but I think it is better
> > this way.
> > It would indeed fire on this pathological case, but I don't think we should
> > care about cases like this, because no one is writing code like this (and
> > if he would then it would probably be a bug).
>
> It would be a known false-positive for a check designed to conform to a
> particular coding standard. When deviations have come up in the past for
> various coding standards, we've added an option to enable the additional
> functionality, which I don't think would be reasonable in this case.
> Alternatively, the CERT guideline could be modified, but that is unlikely to
> occur because binding the function pointer is not a security concern (only
> calling the function).
In case you let binding to function pointer you introduce potential false
negatives which is worse in this case in my opinion.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
Prazek wrote:
> aaron.ballman wrote:
> > Prazek wrote:
> > > aaron.ballman wrote:
> > > > This should be looking at a callExpr() rather than a declRefExpr(),
> > > > should it not?
> > > I was also thinking about this, but this is actually better, because it
> > > will also match with binding rand with function pointer.
> > True, but a DeclRefExpr doesn't mean it's a function call. Binding the
> > function is not contrary to the CERT rule, just calling it. For instance,
> > the following pathological case will be caught by this check:
> > ```
> > if (std::rand) {}
> > ```
> > The behavior of this check should be consistent with cert-env33-c, which
> > only looks at calls. (If we really care about bound functions, we'd need
> > flow control analysis, and I think that's overkill for both of those
> > checks, but wouldn't be opposed to someone writing the flow analysis if
> > they really wanted to.)
> It would indeed fire on this pathological case, but I don't think we should
> care about cases like this, because no one is writing code like this (and if
> he would then it would probably be a bug).
> I don't think that there is much code that binds pointer to std::rand either,
> but I think it would be good to display warning for this, because even if the
> function would be never called, then it means that this is a bug, and if it
> would be called then it would be nice to tell user that rand might be used
> here.
>
> Anyway I don't oppose for changing it to callExpr, but I think it is better
> this way.
> It would indeed fire on this pathological case, but I don't think we should
> care about cases like this, because no one is writing code like this (and if
> he would then it would probably be a bug).
It would be a known false-positive for a check designed to conform to a
particular coding standard. When deviations have come up in the past for
various coding standards, we've added an option to enable the additional
functionality, which I don't think would be reasonable in this case.
Alternatively, the CERT guideline could be modified, but that is unlikely to
occur because binding the function pointer is not a security concern (only
calling the function).
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
falho added a comment. Hi! Thanks for the reviews! I will be off for a few days so I will start working on it when Im back. Greetz! Benedek Repository: rL LLVM https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
Prazek added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
aaron.ballman wrote:
> Prazek wrote:
> > aaron.ballman wrote:
> > > This should be looking at a callExpr() rather than a declRefExpr(),
> > > should it not?
> > I was also thinking about this, but this is actually better, because it
> > will also match with binding rand with function pointer.
> True, but a DeclRefExpr doesn't mean it's a function call. Binding the
> function is not contrary to the CERT rule, just calling it. For instance, the
> following pathological case will be caught by this check:
> ```
> if (std::rand) {}
> ```
> The behavior of this check should be consistent with cert-env33-c, which only
> looks at calls. (If we really care about bound functions, we'd need flow
> control analysis, and I think that's overkill for both of those checks, but
> wouldn't be opposed to someone writing the flow analysis if they really
> wanted to.)
It would indeed fire on this pathological case, but I don't think we should
care about cases like this, because no one is writing code like this (and if he
would then it would probably be a bug).
I don't think that there is much code that binds pointer to std::rand either,
but I think it would be good to display warning for this, because even if the
function would be never called, then it means that this is a bug, and if it
would be called then it would be nice to tell user that rand might be used here.
Anyway I don't oppose for changing it to callExpr, but I think it is better
this way.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
Prazek wrote:
> aaron.ballman wrote:
> > This should be looking at a callExpr() rather than a declRefExpr(), should
> > it not?
> I was also thinking about this, but this is actually better, because it will
> also match with binding rand with function pointer.
True, but a DeclRefExpr doesn't mean it's a function call. Binding the function
is not contrary to the CERT rule, just calling it. For instance, the following
pathological case will be caught by this check:
```
if (std::rand) {}
```
The behavior of this check should be consistent with cert-env33-c, which only
looks at calls. (If we really care about bound functions, we'd need flow
control analysis, and I think that's overkill for both of those checks, but
wouldn't be opposed to someone writing the flow analysis if they really wanted
to.)
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
Prazek added inline comments.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
aaron.ballman wrote:
> This should be looking at a callExpr() rather than a declRefExpr(), should it
> not?
I was also thinking about this, but this is actually better, because it will
also match with binding rand with function pointer.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
Thank you for working on this check!
Comment at: clang-tidy/cert/CERTTidyModule.cpp:37
@@ -35,1 +36,3 @@
// DCL
+CheckFactories.registerCheck(
+"cert-msc50-cpp");
Please place this under a // MSC heading rather than // DCL.
This check should additionally be listed as cert-msc30-c
(https://www.securecoding.cert.org/confluence/display/c/MSC30-C.+Do+not+use+the+rand%28%29+function+for+generating+pseudorandom+numbers).
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0
+ .bind("randomGenerator"),
This should be looking at a callExpr() rather than a declRefExpr(), should it
not?
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:31
@@ +30,3 @@
+ Result.Nodes.getNodeAs("randomGenerator");
+ diag(MatchedDecl->getLocation(), "rand() function has limited randomness, "
+ "use C++11 random library instead");
Prazek wrote:
> I am not sure about the diagnostics convention, it is possible that you
> should replace comma with semicolon.
This diagnostic is incorrect for C code. Should only suggest C++ in
C++ mode. Also, it should be a semicolon rather than a comma in the diagnostic
text.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.h:19
@@ +18,3 @@
+
+/// Pseudorandom number generators are not genuinely random. This checker warns
+/// for the usage of std::rand() function.
Both sentences are correct, but it doesn't clarify why `std::rand()` is bad.
Should add a sentence between these two that says something about why
std::rand() in particular is called out rather than the other pseudorandom
number generators.
Comment at: docs/clang-tidy/checks/cert-msc50-cpp.rst:6
@@ +5,2 @@
+
+Pseudorandom number generators use mathematical algorithms to produce a
sequence of numbers with good statistical properties, but the numbers produced
are not genuinely random. This checker warns for the usage of std::rand().
Eugene.Zelenko wrote:
> Please use check and highlight std::rand() with ``.
The same argument could be used to disallow C++ generators that aren't
true truly random sources. Should specify more about why rand() is particularly
bad.
Comment at: docs/clang-tidy/checks/list.rst:20
@@ -19,2 +19,3 @@
cert-flp30-c
+ cert-msc50-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init)
Please also add a cert-msc30-c file with a redirect (like fio38-c from above).
Comment at: test/clang-tidy/cert-limited-randomness.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s cert-msc50-cpp %t
+
Please also add a test for C since the check works there as well, and will have
different diagnostic text.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
Prazek accepted this revision.
Prazek added a comment.
This revision is now accepted and ready to land.
LGTM with the fixes of docs.
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:31
@@ +30,3 @@
+ Result.Nodes.getNodeAs("randomGenerator");
+ diag(MatchedDecl->getLocation(), "rand() function has limited randomness, "
+ "use C++11 random library instead");
I am not sure about the diagnostics convention, it is possible that you should
replace comma with semicolon.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )
Eugene.Zelenko added a comment. Please mention this check in docs/ReleaseNotes.rst (in alphabetical order). Comment at: docs/clang-tidy/checks/cert-msc50-cpp.rst:4 @@ +3,3 @@ +cert-msc-50 +=== + Should be same length as section name above. Comment at: docs/clang-tidy/checks/cert-msc50-cpp.rst:6 @@ +5,2 @@ + +Pseudorandom number generators use mathematical algorithms to produce a sequence of numbers with good statistical properties, but the numbers produced are not genuinely random. This checker warns for the usage of std::rand(). Please use check and highlight std::rand() with ``. Repository: rL LLVM https://reviews.llvm.org/D22346 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
