[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso updated this revision to Diff 254420.
Charusso marked 4 inline comments as done.
Charusso added a comment.
- Simplify tests.
- Remove dead code, they are far away to being used.
- Add an extra test case.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
Files:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/AllocationState.h
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator.h
clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp
clang/test/Analysis/cert/str31-c-notes.cpp
clang/test/Analysis/cert/str31-c.cpp
Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN: -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+ char buf[BUFFER_SIZE];
+ if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+ }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buff[BUFFERSIZE];
+
+ if (fgets(buff, sizeof(buff), stdin)) {
+ }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+ char buf[128];
+ sprintf(buf, "%s.txt", name);
+ // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+ char buff[128];
+ snprintf(buff, sizeof(buff), "%s.txt", name);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buf[BUF_LENGTH];
+ fscanf(stdin, "%s", buf);
+ // expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buff[BUF_LENGTH];
+ fscanf(stdin, "%1023s", buff);
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char prog_name[128];
+ strcpy(prog_name, name);
+ // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+ return 0;
+}
+
+void func(void) {
+ char buff[256];
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+ }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char *prog_name2 = (char *)malloc(strlen(name) + 1);
+ if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+ }
+
+ free(prog_name2);
+ return 0;
+}
+
+void func(void) {
+ char *buff2;
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+ strcpy(buff2, editor);
+}
+
+free(buff2);
+ }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+ }
+ dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+ }
+ dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buf[BUFFERSIZE];
+ char *p;
+ int ch;
+ p = buf;
+ while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+ }
+ *p++ = 0;
+ if (ch == EOF) {
+/* Handle EOF or error */
+ }
+}
+} // namespace test_getchar_bad
+
+namespace test_getchar_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buf[BUFFERSIZE];
+ int ch;
+ size_t index = 0;
+ size_t
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso updated this revision to Diff 254423.
Charusso added a comment.
- Remove the last dead comment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
Files:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/AllocationState.h
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator.h
clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp
clang/test/Analysis/cert/str31-c-notes.cpp
clang/test/Analysis/cert/str31-c.cpp
Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN: -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+ char buf[BUFFER_SIZE];
+ if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+ }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buff[BUFFERSIZE];
+
+ if (fgets(buff, sizeof(buff), stdin)) {
+ }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+ char buf[128];
+ sprintf(buf, "%s.txt", name);
+ // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+ char buff[128];
+ snprintf(buff, sizeof(buff), "%s.txt", name);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buf[BUF_LENGTH];
+ fscanf(stdin, "%s", buf);
+ // expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buff[BUF_LENGTH];
+ fscanf(stdin, "%1023s", buff);
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char prog_name[128];
+ strcpy(prog_name, name);
+ // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+ return 0;
+}
+
+void func(void) {
+ char buff[256];
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+ }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char *prog_name2 = (char *)malloc(strlen(name) + 1);
+ if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+ }
+
+ free(prog_name2);
+ return 0;
+}
+
+void func(void) {
+ char *buff2;
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+ strcpy(buff2, editor);
+}
+
+free(buff2);
+ }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+ }
+ dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+ }
+ dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buf[BUFFERSIZE];
+ char *p;
+ int ch;
+ p = buf;
+ while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+ }
+ *p++ = 0;
+ if (ch == EOF) {
+/* Handle EOF or error */
+ }
+}
+} // namespace test_getchar_bad
+
+namespace test_getchar_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buf[BUFFERSIZE];
+ int ch;
+ size_t index = 0;
+ size_t chars_read = 0;
+
+ while ((ch = getchar()) != '\n' && ch != EOF) {
+if (index < sizeof(buf) - 1) {
+
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso added a comment.
Thanks for the review, hopefully if I ping @NoQ in every round, it will be
green-marked soon.
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:55
+ // they can cause a not null-terminated string. In this case we store the
+ // string is being not null-terminated in the 'NullTerminationMap'.
+ //
balazske wrote:
> The functions `gets`, `strcpy` return a null-terminated string according to
> standard, probably `sprintf` too, and the `fscanf` `%s` output is
> null-terminated too even if no length is specified (maybe it writes outside
> of the specified buffer but null terminated).
From where do you observe such information? I have not seen anything
`strcpy()`-specific in the standard. My observation clearly says that, if there
is not enough space for the data you *could* even write to overlapping memory,
or as people says, you could meet nasal demons. It is called *undefined*
behavior.
Comment at: clang/test/Analysis/cert/str31-c.cpp:117
+ if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
balazske wrote:
> Do we get a warning if the `+1` above is missing?
Test added.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
balazske added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:55
+ // they can cause a not null-terminated string. In this case we store the
+ // string is being not null-terminated in the 'NullTerminationMap'.
+ //
The functions `gets`, `strcpy` return a null-terminated string according to
standard, probably `sprintf` too, and the `fscanf` `%s` output is
null-terminated too even if no length is specified (maybe it writes outside of
the specified buffer but null terminated).
Comment at: clang/test/Analysis/cert/str31-c.cpp:117
+ if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
Do we get a warning if the `+1` above is missing?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso added a comment. Given that the secondary behavior confuse people I have removed it for now. May if someone introduce a NullTerminationChecker then we introduce such option to warn on insecure calls instant. Thanks @balazske for influencing that change. @NoQ this project had a deadline like half year ago, could you smash that green button please? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70411/new/ https://reviews.llvm.org/D70411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso updated this revision to Diff 254061.
Charusso marked 6 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.
- Get rid of the secondary behavior for now.
- Fix review comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
Files:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/AllocationState.h
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator.h
clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp
clang/test/Analysis/cert/str31-c-notes.cpp
clang/test/Analysis/cert/str31-c.cpp
Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN: -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+void do_something(char *buffer);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+ char buf[BUFFER_SIZE];
+ if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+ }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buff[BUFFERSIZE];
+
+ if (fgets(buff, sizeof(buff), stdin)) {
+ }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+ char buf[128];
+ sprintf(buf, "%s.txt", name);
+ // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+ char buff[128];
+ snprintf(buff, sizeof(buff), "%s.txt", name);
+
+ do_something(buff);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buf[BUF_LENGTH];
+ if (fscanf(stdin, "%s", buf)) {
+// expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+ }
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buff[BUF_LENGTH];
+ if (fscanf(stdin, "%1023s", buff)) {
+do_something(buff);
+ }
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char prog_name[128];
+ strcpy(prog_name, name);
+ // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+
+ return 0;
+}
+
+void func(void) {
+ char buff[256];
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+ }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char *prog_name2 = (char *)malloc(strlen(name) + 1);
+ if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+ }
+
+ do_something(prog_name2);
+
+ free(prog_name2);
+ return 0;
+}
+
+void func(void) {
+ char *buff2;
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+ strcpy(buff2, editor);
+}
+
+do_something(buff2);
+free(buff2);
+ }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+ }
+ dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+ }
+ dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buf[BUFFERSIZE];
+ char *p;
+ int ch;
+ p = buf;
+ while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+ }
+ *p++ = 0;
+ if (ch == EOF) {
+/* Handle EOF or error */
+ }
+}
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso marked 6 inline comments as done. Charusso added a comment. "To prevent such errors, either limit copies through truncation or, preferably, ensure that the destination is of sufficient size to hold the character data" - from the rule's page. Most of the projects are fine truncating by hand because the write happens in somewhat well-bounded strings: IP-addresses, names, numbers... I wanted to make this as practical as possible. Until you are having a null-terminated string without being read, you are most likely fine. Feel free to try this out, probably you would already understand the `WarnOnCall` option very well. Comment at: clang/docs/analyzer/checkers.rst:1935 + +alpha.security.cert.str.31c +""" balazske wrote: > Charusso wrote: > > Szelethus wrote: > > > balazske wrote: > > > > There are already more checkers that can check for CERT related > > > > problems but not specially made for these. These checkers do not reside > > > > in this new `cert` group. And generally a checker does not check for > > > > specifically a CERT rule, instead for more of them or other things too, > > > > or more checkers can detect a single rule. (And the user can think that > > > > only these CERT rules are checkable that exist in this package, that is > > > > not true.) So I do not like the introduction of this new `cert` > > > > package. (The documentation of existing checkers lists if the checker > > > > is designed for a CERT rule.) > > > I disagree to some extent. I think it would be great to have a `cert` > > > package that houses all checkers for each of the rules with the addition > > > of checker aliases. Clang-tidy has something similar as well! > > I designed the checker only for the rule STR31-C that is why I have picked > > package `cert`. Clang-Tidy could aliasing checks. For example the check > > could be in the `bugprone` category aliased to `cert` and both could > > trigger the analysis. > > > > > the user can think that only these CERT rules are checkable > > We only need to move `security.FloatLoopCounter` under the `cert` package. > > What else SEI CERT rules are finished off other than package `cert`? It is > > not my fault if someone could not package well or could not catch most of > > the issues of a SEI CERT rule or could not reach the SEI CERT group to note > > the fact the Analyzer catch a very tiny part of a rule. However, this patch > > package well and could catch most of the STR31-C rule. > So I can move this checker: D71510 into `alpha.security.cert.err.33c`? Well, not exactly. No one should care about alpha checkers except advanced Static Analyzer developers like you do right now. Since alpha is a dead-zone you can put your work anywhere. The core issue was Ted's placing of CERT rules. I have written the first CERT-checker after Ted's, which introduced such packaging, then it became dead, so I have lost interest to make the package-aliasing a thing. If you wish to move to `cert`, feel free, but please note that, your first idea of bad-packaging is right. We really need the functionality of package-aliasing but I have ran out of time and the project died as well. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803 + +} // end "cert.str" + balazske wrote: > Charusso wrote: > > balazske wrote: > > > NoQ wrote: > > > > `alpha.cert.str`. > > > This text may be hard to understand. The option means "if it is off, the > > > problem report happens only if there is no hand-written null termination > > > of the string"? I looked at the CERT rule but did not find out why is > > > null termination by hand important here. (I did not look at the code to > > > find out what it does.) And "WarnOnCall" suggests that there is warning > > > made on calls if on, or not at all or at other location if off, is this > > > correct? > > You let the buffer overflow by a bugprone function call, then you adjust > > the null-terminator by simply `buf[size] = '\0'`, then you made sure the > > buffer cannot overflow, since you have terminated it. It is a very common > > idiom therefore I do not think a hand-written null-termination is a > > security issue. The SEI CERT rules are all theoretical so you will not find > > anything useful in practice. My solution is practical. > > > > > This text may be hard to understand. > > Please note that this text only made for Static Analyzer developers. Let us > > rephrase it then: > > > > > Whether the checker needs to warn on the bugprone function calls > > > immediately or look for bugprone hand-written null-termination of > > > bugprone function call made strings. It is a common idiom to > > > null-terminate the string by hand after the insecure function call > > > produce the string which could be misused so that it is on by default. It > > > is useful to turn it off to reduce the noise of the checker,
[PATCH] D70411: [analyzer] CERT: STR31-C
balazske added inline comments.
Comment at: clang/docs/analyzer/checkers.rst:1935
+
+alpha.security.cert.str.31c
+"""
Charusso wrote:
> Szelethus wrote:
> > balazske wrote:
> > > There are already more checkers that can check for CERT related problems
> > > but not specially made for these. These checkers do not reside in this
> > > new `cert` group. And generally a checker does not check for specifically
> > > a CERT rule, instead for more of them or other things too, or more
> > > checkers can detect a single rule. (And the user can think that only
> > > these CERT rules are checkable that exist in this package, that is not
> > > true.) So I do not like the introduction of this new `cert` package. (The
> > > documentation of existing checkers lists if the checker is designed for a
> > > CERT rule.)
> > I disagree to some extent. I think it would be great to have a `cert`
> > package that houses all checkers for each of the rules with the addition of
> > checker aliases. Clang-tidy has something similar as well!
> I designed the checker only for the rule STR31-C that is why I have picked
> package `cert`. Clang-Tidy could aliasing checks. For example the check could
> be in the `bugprone` category aliased to `cert` and both could trigger the
> analysis.
>
> > the user can think that only these CERT rules are checkable
> We only need to move `security.FloatLoopCounter` under the `cert` package.
> What else SEI CERT rules are finished off other than package `cert`? It is
> not my fault if someone could not package well or could not catch most of the
> issues of a SEI CERT rule or could not reach the SEI CERT group to note the
> fact the Analyzer catch a very tiny part of a rule. However, this patch
> package well and could catch most of the STR31-C rule.
So I can move this checker: D71510 into `alpha.security.cert.err.33c`?
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+
Charusso wrote:
> balazske wrote:
> > NoQ wrote:
> > > `alpha.cert.str`.
> > This text may be hard to understand. The option means "if it is off, the
> > problem report happens only if there is no hand-written null termination of
> > the string"? I looked at the CERT rule but did not find out why is null
> > termination by hand important here. (I did not look at the code to find out
> > what it does.) And "WarnOnCall" suggests that there is warning made on
> > calls if on, or not at all or at other location if off, is this correct?
> You let the buffer overflow by a bugprone function call, then you adjust the
> null-terminator by simply `buf[size] = '\0'`, then you made sure the buffer
> cannot overflow, since you have terminated it. It is a very common idiom
> therefore I do not think a hand-written null-termination is a security issue.
> The SEI CERT rules are all theoretical so you will not find anything useful
> in practice. My solution is practical.
>
> > This text may be hard to understand.
> Please note that this text only made for Static Analyzer developers. Let us
> rephrase it then:
>
> > Whether the checker needs to warn on the bugprone function calls
> > immediately or look for bugprone hand-written null-termination of bugprone
> > function call made strings. It is a common idiom to null-terminate the
> > string by hand after the insecure function call produce the string which
> > could be misused so that it is on by default. It is useful to turn it off
> > to reduce the noise of the checker, because people usually null-terminate
> > the string by hand immediately after the bugprone function call produce the
> > string.
>
> Do you buy that?
First sentence is OK, the later part relatively better. Probably the
explanations about why it is good to turn it on or off can be left out, these
are misunderstandable. (If the manual null-termination is done, in one case it
is a "common idiom but can be misused so make warning" on other case "these
warnings generates only extra noise".)
It can be something like this (if correct):
```
Whether the checker wars on the function calls immediately or warns on bugprone
hand-written null-termination of function call made strings. Default value is
on because null-terminating the string by hand after the insecure function call
could be misused. But it is a common idiom to manually null-terminate the
strings immediately after the function calls so it could be turned off to
reduce noise of the checker.
```
I do not like the word "bugprone", is it correct english? ("Problematic" or
"insecure" could be used instead.)
Comment at: clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp:68
+
+ do_something(dest);
+ // expected-warning@-1 {{'dest' is not null-terminated}}
Charusso wrote:
> balazske wrote:
> > Maybe I have wrong expectations about what this checker does but
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso marked 4 inline comments as done.
Charusso added inline comments.
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+
balazske wrote:
> NoQ wrote:
> > `alpha.cert.str`.
> This text may be hard to understand. The option means "if it is off, the
> problem report happens only if there is no hand-written null termination of
> the string"? I looked at the CERT rule but did not find out why is null
> termination by hand important here. (I did not look at the code to find out
> what it does.) And "WarnOnCall" suggests that there is warning made on calls
> if on, or not at all or at other location if off, is this correct?
You let the buffer overflow by a bugprone function call, then you adjust the
null-terminator by simply `buf[size] = '\0'`, then you made sure the buffer
cannot overflow, since you have terminated it. It is a very common idiom
therefore I do not think a hand-written null-termination is a security issue.
The SEI CERT rules are all theoretical so you will not find anything useful in
practice. My solution is practical.
> This text may be hard to understand.
Please note that this text only made for Static Analyzer developers. Let us
rephrase it then:
> Whether the checker needs to warn on the bugprone function calls immediately
> or look for bugprone hand-written null-termination of bugprone function call
> made strings. It is a common idiom to null-terminate the string by hand after
> the insecure function call produce the string which could be misused so that
> it is on by default. It is useful to turn it off to reduce the noise of the
> checker, because people usually null-terminate the string by hand immediately
> after the bugprone function call produce the string.
Do you buy that?
Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:28
+/// \returns The MallocBugVisitor.
+std::unique_ptr getMallocBRVisitor(SymbolRef Sym);
balazske wrote:
> This documentation could be improved or left out.
Totally, I have already forgotten I wanted to remove it. Thanks!
Comment at: clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp:68
+
+ do_something(dest);
+ // expected-warning@-1 {{'dest' is not null-terminated}}
balazske wrote:
> Maybe I have wrong expectations about what this checker does but did
> understand it correctly yet. The main problem here is that `dest` may be not
> large enough (if size of `src` is not known). This would be a better text for
> the error message? And if this happens (the write outside) it is already a
> fatal error, can cause crash. If no buffer overflow happens the terminating
> zero is copied from `src`, so why would `dst` be not null terminated?
I like that error message because that is the truth, that is the issue. If it
would crash so the operating system would detect a fatal error we should not
develop Static Analyzer and vulnerabilities would not exist. That would be
cool, btw.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
balazske added inline comments.
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+
NoQ wrote:
> `alpha.cert.str`.
This text may be hard to understand. The option means "if it is off, the
problem report happens only if there is no hand-written null termination of the
string"? I looked at the CERT rule but did not find out why is null termination
by hand important here. (I did not look at the code to find out what it does.)
And "WarnOnCall" suggests that there is warning made on calls if on, or not at
all or at other location if off, is this correct?
Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:28
+/// \returns The MallocBugVisitor.
+std::unique_ptr getMallocBRVisitor(SymbolRef Sym);
This documentation could be improved or left out.
Comment at: clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp:68
+
+ do_something(dest);
+ // expected-warning@-1 {{'dest' is not null-terminated}}
Maybe I have wrong expectations about what this checker does but did understand
it correctly yet. The main problem here is that `dest` may be not large enough
(if size of `src` is not known). This would be a better text for the error
message? And if this happens (the write outside) it is already a fatal error,
can cause crash. If no buffer overflow happens the terminating zero is copied
from `src`, so why would `dst` be not null terminated?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso marked 5 inline comments as done. Charusso added a comment. Thanks for the feedback! Given that it will remain an `alpha` checker for a long time (~1 year), no one really should use it. Comment at: clang/docs/analyzer/checkers.rst:1935 + +alpha.security.cert.str.31c +""" Szelethus wrote: > balazske wrote: > > There are already more checkers that can check for CERT related problems > > but not specially made for these. These checkers do not reside in this new > > `cert` group. And generally a checker does not check for specifically a > > CERT rule, instead for more of them or other things too, or more checkers > > can detect a single rule. (And the user can think that only these CERT > > rules are checkable that exist in this package, that is not true.) So I do > > not like the introduction of this new `cert` package. (The documentation of > > existing checkers lists if the checker is designed for a CERT rule.) > I disagree to some extent. I think it would be great to have a `cert` package > that houses all checkers for each of the rules with the addition of checker > aliases. Clang-tidy has something similar as well! I designed the checker only for the rule STR31-C that is why I have picked package `cert`. Clang-Tidy could aliasing checks. For example the check could be in the `bugprone` category aliased to `cert` and both could trigger the analysis. > the user can think that only these CERT rules are checkable We only need to move `security.FloatLoopCounter` under the `cert` package. What else SEI CERT rules are finished off other than package `cert`? It is not my fault if someone could not package well or could not catch most of the issues of a SEI CERT rule or could not reach the SEI CERT group to note the fact the Analyzer catch a very tiny part of a rule. However, this patch package well and could catch most of the STR31-C rule. Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h:22 +extern const char *const CXXObjectLifecycle; +extern const char *const SecurityError; +} // namespace categories balazske wrote: > Are there already not other checkers that find security related bugs (the > taint checker?)? Why do these not use a `SecurityError`? It is not bad to > have a `SecurityError` but maybe there is a reason why was it not there > already. If these categories are exclusive it is hard to find out what > problem (probably already existing bug type in other checkers) belongs to > what category (it can be for this checker `UnixAPI` or `MemoryError` too?). We have only three pure security checkers: `security.insecureAPI`, `security.FloatLoopCounter` (FLP30-C, FLP30-CPP) and `alpha.security.cert.pos.34c`. The non-alpha checkers are very old, but Ted definitely made that category: ``` const char *bugType = "Floating point variable used as loop counter"; BR.EmitBasicReport(bugType, "Security", os.str().c_str(), FS->getLocStart(), ranges.data(), ranges.size()); ``` - https://github.com/llvm/llvm-project/commit/9c49762776b4d2cb16911dec0c873c90a6508968 Every other security-ish checker does not catch the CERT rule's examples. May if the checker evolves I would pick MemoryError, but it will not evolve soon. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70411/new/ https://reviews.llvm.org/D70411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
Szelethus added inline comments. Comment at: clang/docs/analyzer/checkers.rst:1935 + +alpha.security.cert.str.31c +""" balazske wrote: > There are already more checkers that can check for CERT related problems but > not specially made for these. These checkers do not reside in this new `cert` > group. And generally a checker does not check for specifically a CERT rule, > instead for more of them or other things too, or more checkers can detect a > single rule. (And the user can think that only these CERT rules are checkable > that exist in this package, that is not true.) So I do not like the > introduction of this new `cert` package. (The documentation of existing > checkers lists if the checker is designed for a CERT rule.) I disagree to some extent. I think it would be great to have a `cert` package that houses all checkers for each of the rules with the addition of checker aliases. Clang-tidy has something similar as well! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70411/new/ https://reviews.llvm.org/D70411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
balazske added inline comments. Comment at: clang/docs/analyzer/checkers.rst:1935 + +alpha.security.cert.str.31c +""" There are already more checkers that can check for CERT related problems but not specially made for these. These checkers do not reside in this new `cert` group. And generally a checker does not check for specifically a CERT rule, instead for more of them or other things too, or more checkers can detect a single rule. (And the user can think that only these CERT rules are checkable that exist in this package, that is not true.) So I do not like the introduction of this new `cert` package. (The documentation of existing checkers lists if the checker is designed for a CERT rule.) Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h:22 +extern const char *const CXXObjectLifecycle; +extern const char *const SecurityError; +} // namespace categories Are there already not other checkers that find security related bugs (the taint checker?)? Why do these not use a `SecurityError`? It is not bad to have a `SecurityError` but maybe there is a reason why was it not there already. If these categories are exclusive it is hard to find out what problem (probably already existing bug type in other checkers) belongs to what category (it can be for this checker `UnixAPI` or `MemoryError` too?). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70411/new/ https://reviews.llvm.org/D70411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso updated this revision to Diff 241752.
Charusso edited the summary of this revision.
Charusso added a comment.
- 2020-ify the checker writing
- Remove extra bullet-points of CERT checker documentation: we only need the
checker's documentation, not the packages.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
Files:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
clang/lib/StaticAnalyzer/Checkers/AllocationState.h
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
clang/test/Analysis/Inputs/system-header-simulator.h
clang/test/Analysis/analyzer-config.c
clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp
clang/test/Analysis/cert/str31-c-notes-warn-on-call-on.cpp
clang/test/Analysis/cert/str31-c-warn-on-call-off.cpp
clang/test/Analysis/cert/str31-c-warn-on-call-on.cpp
clang/test/Analysis/cert/str31-c.cpp
Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN: -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+void do_something(char *buffer);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+ char buf[BUFFER_SIZE];
+ if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+ }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buff[BUFFERSIZE];
+
+ if (fgets(buff, sizeof(buff), stdin)) {
+ }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+ char buf[128];
+ sprintf(buf, "%s.txt", name);
+ // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+ char buff[128];
+ snprintf(buff, sizeof(buff), "%s.txt", name);
+
+ do_something(buff);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buf[BUF_LENGTH];
+ if (fscanf(stdin, "%s", buf)) {
+// expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+ }
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buff[BUF_LENGTH];
+ if (fscanf(stdin, "%1023s", buff)) {
+do_something(buff);
+ }
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char prog_name[128];
+ strcpy(prog_name, name);
+ // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+
+ return 0;
+}
+
+void func(void) {
+ char buff[256];
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+ }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char *prog_name2 = (char *)malloc(strlen(name) + 1);
+ if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+ }
+
+ do_something(prog_name2);
+
+ free(prog_name2);
+ return 0;
+}
+
+void func(void) {
+ char *buff2;
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+ strcpy(buff2, editor);
+}
+
+do_something(buff2);
+free(buff2);
+ }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+ }
+ dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i]
[PATCH] D70411: [analyzer] CERT: STR31-C
Charusso updated this revision to Diff 233903.
Charusso retitled this revision from "[analyzer] CERT: StrChecker: Implementing
most of the STR31-C" to "[analyzer] CERT: STR31-C".
Charusso added a comment.
- Iterate over parameters rather arguments for searching string-reading.
- Better notes of the argument's index.
- Avoid `C.getPredecessor()`, use the error-node instead.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
Files:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
clang/lib/StaticAnalyzer/Checkers/AllocationState.h
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
clang/test/Analysis/Inputs/system-header-simulator.h
clang/test/Analysis/analyzer-config.c
clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp
clang/test/Analysis/cert/str31-c-notes-warn-on-call-on.cpp
clang/test/Analysis/cert/str31-c-warn-on-call-off.cpp
clang/test/Analysis/cert/str31-c-warn-on-call-on.cpp
clang/test/Analysis/cert/str31-c.cpp
Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN: -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+void do_something(char *buffer);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+ char buf[BUFFER_SIZE];
+ if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+ }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+ char buff[BUFFERSIZE];
+
+ if (fgets(buff, sizeof(buff), stdin)) {
+ }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+ char buf[128];
+ sprintf(buf, "%s.txt", name);
+ // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+ char buff[128];
+ snprintf(buff, sizeof(buff), "%s.txt", name);
+
+ do_something(buff);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buf[BUF_LENGTH];
+ if (fscanf(stdin, "%s", buf)) {
+// expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+ }
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+ char buff[BUF_LENGTH];
+ if (fscanf(stdin, "%1023s", buff)) {
+do_something(buff);
+ }
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char prog_name[128];
+ strcpy(prog_name, name);
+ // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+
+ return 0;
+}
+
+void func(void) {
+ char buff[256];
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+ }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+ const char *const name = (argc && argv[0]) ? argv[0] : "";
+ char *prog_name2 = (char *)malloc(strlen(name) + 1);
+ if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+ }
+
+ do_something(prog_name2);
+
+ free(prog_name2);
+ return 0;
+}
+
+void func(void) {
+ char *buff2;
+ char *editor = getenv("EDITOR");
+ if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+ strcpy(buff2, editor);
+}
+
+do_something(buff2);
+free(buff2);
+ }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+ size_t i;
+
+ for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+ }
