Charusso updated this revision to Diff 150643.
Charusso marked 4 inline comments as done.
Charusso added a comment.
`memchr()` revision: it is problematic if the second argument is `'\0'`, there
is no other case. Added a new type of matcher, to match function calls. More
static functions and test cases and huge refactor.
https://reviews.llvm.org/D45050
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
clang-tidy/bugprone/NotNullTerminatedResultCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
===================================================================
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
@@ -0,0 +1,150 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef unsigned int wchar_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+wchar_t *wcschr(const wchar_t *, int);
+errno_t *wcsncpy_s(wchar_t *, const wchar_t *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *wmemcpy(void *, const void *, size_t);
+errno_t wmemcpy_s(void *, size_t, const void *, size_t);
+void *wmemchr(const void *, int, size_t);
+void *wmemmove(void *, const void *, size_t);
+errno_t wmemmove_s(void *, size_t, const void *, size_t);
+void *wmemset(void *, int, size_t);
+
+int wcsncmp(const wchar_t *, const wchar_t *, size_t);
+size_t wcsxfrm(wchar_t *, const wchar_t *, size_t);
+
+
+void bad_alloca(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)alloca(wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: alloca(wcslen(src) + 1);
+}
+
+void good_alloca(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)alloca(wcslen(src) + 1);
+}
+
+void bad_calloc(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)calloc(wcslen(src), sizeof(wchar_t));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void good_calloc(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void bad_malloc(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)malloc(wcslen(src) * 2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: malloc((wcslen(src) * 2) + 1);
+}
+
+void good_malloc(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)malloc((wcslen(src) * 2) + 1);
+}
+
+void bad_realloc(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)realloc(dest, (wcslen(dest) + wcslen(src)));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void good_realloc(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void bad_wmemcpy(wchar_t *dest, const wchar_t *src) {
+ wmemcpy(dest, src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_safe(wchar_t *dest, const wchar_t *src) {
+ wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+ wmemcpy_s(dest, wcslen(dest), src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+ wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemchr(wchar_t *dest, const wchar_t *src) {
+ dest = (wchar_t *)wmemchr(src, '\0', wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: the length is too short for the last '\0' [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wcschr(src, '\0');
+}
+
+void good_wmemchr(wchar_t *dest, const wchar_t *src) {
+ dest = wcschr(src, '\0');
+}
+
+void bad_wmemmove(wchar_t *dest, const wchar_t *src) {
+ wmemmove(dest, src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemmove' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1));
+}
+
+void good_wmemmove_safe(wchar_t *dest, const wchar_t *src) {
+ wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1));
+}
+
+void bad_wmemmove_s(wchar_t *dest, const wchar_t *src) {
+ wmemmove_s(dest, wcslen(dest), src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemmove_s' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1));
+}
+
+void good_wmemmove_s_1(wchar_t *dest, const wchar_t *src) {
+ wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1));
+}
+
+void good_wmemmove_s_2(wchar_t *dest, const wchar_t *src) {
+ wmemmove_s(dest, wcslen(dest), (src + 1), wcslen(src));
+}
+
+void bad_wmemset(wchar_t *dest, const wchar_t *src) {
+ wmemset(dest, '-', (wcslen(src) + 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemset' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wmemset(dest, '-', wcslen(src));
+}
+
+void good_wmemset(wchar_t *dest, const wchar_t *src) {
+ wmemset(dest, '-', wcslen(src));
+}
+
+int bad_wcsncmp(const wchar_t *str1, const wchar_t *str2) {
+ return wcsncmp(str1, str2, (wcslen(str1) + 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wcsncmp(str1, str2, wcslen(str1));
+}
+
+int good_wcsncmp(const wchar_t *str1, const wchar_t *str2) {
+ return wcsncmp(str1, str2, wcslen(str1));
+}
+
+void bad_wcsxfrm(wchar_t *dest, const wchar_t *src) {
+ wcsxfrm(dest, src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wcsxfrm' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wcsxfrm(dest, src, (wcslen(src) + 1));
+}
+
+void good_wcsxfrm(wchar_t *dest, const wchar_t *src) {
+ wcsxfrm(dest, src, (wcslen(src) + 1));
+}
+
Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
===================================================================
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: AreSafeFunctionsAvailable, value: 0}]}" \
+// RUN: -- -std=c99
+
+typedef unsigned int size_t;
+size_t wcslen(const char *);
+char *wcsncpy(char *, const char *, size_t);
+
+void *wmemcpy(void *, const void *, size_t);
+void *wmemmove(void *, const void *, size_t);
+
+void bad_wmemcpy(char *dest, const char *src) {
+ wmemcpy(dest, src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wcsncpy(dest, src, (wcslen(src) + 1));
+}
+
+void good_wmemcpy_before_safe(char *dest, const char *src) {
+ wcsncpy(dest, src, (wcslen(src) + 1));
+}
+
+void bad_wmemmove(char *dest, const char *src) {
+ wmemmove(dest, src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemmove' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wmemmove(dest, src, (wcslen(src) + 1));
+}
+
+void good_wmemmove_before_safe_1(char *dest, const char *src) {
+ wmemmove(dest, src, (wcslen(src) + 1));
+}
+
+void good_wmemmove_before_safe_2(char *dest, const char *src) {
+ wmemmove(dest, (src + 1), wcslen(src));
+}
+
Index: test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
===================================================================
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
@@ -0,0 +1,163 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef int errno_t;
+size_t strlen(const char *);
+char *strerror(int);
+char *strchr(const char *, int);
+errno_t *strncpy_s(char *, const char *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *memcpy(void *, const void *, size_t);
+errno_t memcpy_s(void *, size_t, const void *, size_t);
+void *memchr(const void *, int, size_t);
+void *memmove(void *, const void *, size_t);
+errno_t memmove_s(void *, size_t, const void *, size_t);
+void *memset(void *, int, size_t);
+
+errno_t strerror_s(char *, size_t, int);
+int strncmp(const char *, const char *, size_t);
+size_t strxfrm(char *, const char *, size_t);
+
+
+void bad_alloca(char *dest, const char *src) {
+ dest = (char *)alloca(strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: alloca(strlen(src) + 1);
+}
+
+void good_alloca(char *dest, const char *src) {
+ dest = (char *)alloca(strlen(src) + 1);
+}
+
+void bad_calloc(char *dest, const char *src) {
+ dest = (char *)calloc(strlen(src), sizeof(char));
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: calloc((strlen(src) + 1), sizeof(char));
+}
+
+void good_calloc(char *dest, const char *src) {
+ dest = (char *)calloc((strlen(src) + 1), sizeof(char));
+}
+
+void bad_malloc(char *dest, const char *src) {
+ dest = (char *)malloc(strlen(src) * 2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: malloc((strlen(src) * 2) + 1);
+}
+
+void good_malloc(char *dest, const char *src) {
+ dest = (char *)malloc((strlen(src) * 2) + 1);
+}
+
+void bad_realloc(char *dest, const char *src) {
+ dest = (char *)realloc(dest, (strlen(dest) + strlen(src)));
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: realloc(dest, (strlen(dest) + (strlen(src) + 1)));
+}
+
+void good_realloc(char *dest, const char *src) {
+ dest = (char *)realloc(dest, (strlen(dest) + (strlen(src) + 1)));
+}
+
+void bad_memcpy(char *dest, const char *src) {
+ memcpy(dest, src, strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncpy_s(dest, src, strlen(src));
+}
+
+void good_memcpy_safe(char *dest, const char *src) {
+ strncpy_s(dest, src, strlen(src));
+}
+
+void bad_memcpy_s(char *dest, const char *src) {
+ memcpy_s(dest, strlen(dest), src, strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncpy_s(dest, src, strlen(src));
+}
+
+void good_memcpy_s(char *dest, const char *src) {
+ strncpy_s(dest, src, strlen(src));
+}
+
+void bad_memchr(char *dest, const char *src) {
+ dest = (char *)memchr(src, '\0', strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: the length is too short for the last '\0' [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strchr(src, '\0');
+}
+
+void good_memchr(char *dest, const char *src) {
+ dest = strchr(src, '\0');
+}
+
+void bad_memmove(char *dest, const char *src) {
+ memmove(dest, src, strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memmove' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memmove_s(dest, strlen(dest), src, (strlen(src) + 1));
+}
+
+void good_memmove_safe(char *dest, const char *src) {
+ memmove_s(dest, strlen(dest), src, (strlen(src) + 1));
+}
+
+void bad_memmove_s(char *dest, const char *src) {
+ memmove_s(dest, strlen(dest), src, strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memmove_s' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memmove_s(dest, strlen(dest), src, (strlen(src) + 1));
+}
+
+void good_memmove_s_1(char *dest, const char *src) {
+ memmove_s(dest, strlen(dest), src, (strlen(src) + 1));
+}
+
+void good_memmove_s_2(char *dest, const char *src) {
+ memmove_s(dest, strlen(dest), (src + 1), strlen(src));
+}
+
+void bad_memset(char *dest, const char *src) {
+ memset(dest, '-', (strlen(src) + 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memset(dest, '-', strlen(src));
+}
+
+void good_memset(char *dest, const char *src) {
+ memset(dest, '-', strlen(src));
+}
+
+void bad_strerror_s(char *dest, int errno) {
+ strerror_s(dest, strlen(strerror(errno)), errno);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'strerror_s' is not null-terminated and missing the last character of the error message [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strerror_s(dest, (strlen(strerror(errno)) + 1), errno);
+}
+
+void good_strerror_s(char *dest, int errno) {
+ strerror_s(dest, (strlen(strerror(errno)) + 1), errno);
+}
+
+int bad_strncmp(char *str1, const char *str2) {
+ return strncmp(str1, str2, (strlen(str1) + 1));
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncmp(str1, str2, strlen(str1));
+}
+
+int good_strncmp(const char *str1, const char *str2) {
+ return strncmp(str1, str2, strlen(str1));
+}
+
+void bad_strxfrm(char *long_destination_name, const char *long_source_name) {
+ strxfrm(long_destination_name, long_source_name,
+ strlen(long_source_name));
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: the result from calling 'strxfrm' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strxfrm(long_destination_name, long_source_name,
+ // CHECK-FIXES-NEXT: strlen(long_source_name) + 1);
+}
+
+void good_strxfrm(char *long_destination_name, const char *long_source_name) {
+ strxfrm(long_destination_name, long_source_name,
+ strlen(long_source_name) + 1);
+}
Index: test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
===================================================================
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: AreSafeFunctionsAvailable, value: 0}]}" \
+// RUN: -- -std=c99
+
+typedef unsigned int size_t;
+size_t strlen(const char *);
+char *strncpy(char *, const char *, size_t);
+
+void *memcpy(void *, const void *, size_t);
+void *memmove(void *, const void *, size_t);
+
+void bad_memcpy(char *dest, const char *src) {
+ memcpy(dest, src, strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncpy(dest, src, (strlen(src) + 1));
+}
+
+void good_memcpy_before_safe(char *dest, const char *src) {
+ strncpy(dest, src, (strlen(src) + 1));
+}
+
+void bad_memmove(char *dest, const char *src) {
+ memmove(dest, src, strlen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memmove' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memmove(dest, src, (strlen(src) + 1));
+}
+
+void good_memmove_before_safe_1(char *dest, const char *src) {
+ memmove(dest, src, (strlen(src) + 1));
+}
+
+void good_memmove_before_safe_2(char *dest, const char *src) {
+ memmove(dest, (src + 1), strlen(src));
+}
+
Index: test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
===================================================================
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
@@ -0,0 +1,235 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef int errno_t;
+size_t strlen(const char *);
+char *strerror(int);
+char *strchr(const char *, int);
+errno_t *strncpy_s(char *, const char *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *memcpy(void *, const void *, size_t);
+errno_t memcpy_s(void *, size_t, const void *, size_t);
+void *memchr(const void *, int, size_t);
+void *memmove(void *, const void *, size_t);
+errno_t memmove_s(void *, size_t, const void *, size_t);
+void *memset(void *, int, size_t);
+
+errno_t strerror_s(char *, size_t, int);
+int strncmp(const char *, const char *, size_t);
+size_t strxfrm(char *, const char *, size_t);
+
+int getLengthWithoutInc(const char *str) {
+ return strlen(str);
+}
+
+int getLengthWithInc(const char *str) {
+ int length = strlen(str) + 1;
+ return length;
+}
+
+
+void bad_alloca(char *dest, const char *src) {
+ int length = strlen(src);
+ dest = (char *)alloca(length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: dest = (char *)alloca(length + 1);
+}
+
+void good_alloca(char *dest, const char *src) {
+ int length = strlen(src);
+ dest = (char *)alloca(length + 1);
+}
+
+void bad_calloc(char *dest, const char *src) {
+ dest = (char *)calloc(getLengthWithoutInc(src), sizeof(char));
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: dest = (char *)calloc(getLengthWithoutInc(src) + 1, sizeof(char));
+}
+
+void good_calloc(char *dest, const char *src) {
+ dest = (char *)calloc(getLengthWithoutInc(src) + 1, sizeof(char));
+}
+
+void bad_malloc1(char *dest, const char *src) {
+ int length = getLengthWithoutInc(src) * 2;
+ dest = (char *)malloc(length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: dest = (char *)malloc(length + 1);
+}
+
+void good_malloc1(char *dest, const char *src) {
+ int length = getLengthWithoutInc(src) * 2;
+ dest = (char *)malloc(length + 1);
+}
+
+void bad_malloc2(char *dest, const char *src) {
+ int length = strlen(src);
+ dest = (char *)malloc(length * 2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: dest = (char *)malloc(length * 2 + 1);
+}
+
+void good_malloc2(char *dest, const char *src) {
+ int length = strlen(src);
+ dest = (char *)malloc(length * 2 + 1);
+}
+
+void bad_realloc(char *dest, const char *src) {
+ int length = strlen(dest) + strlen(src);
+ dest = (char *)realloc(dest, length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: dest = (char *)realloc(dest, length + 1);
+}
+
+void good_realloc(char *dest, const char *src) {
+ int length = strlen(dest) + strlen(src);
+ dest = (char *)realloc(dest, length + 1);
+}
+
+void bad_memcpy(char *dest, const char *src) {
+ int length = strlen(src);
+ memcpy(dest, src, length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncpy_s(dest, src, length);
+}
+
+void good_memcpy_safe(char *dest, const char *src) {
+ int length = strlen(src);
+ strncpy_s(dest, src, length);
+}
+
+void bad_memcpy_s(char *dest, const char *src) {
+ int destLength = strlen(dest);
+ int srcLength = strlen(src);
+ memcpy_s(dest, destLength, src, srcLength);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncpy_s(dest, src, srcLength);
+}
+
+void good_memcpy_s(char *dest, const char *src) {
+ int length = strlen(src);
+ strncpy_s(dest, src, length);
+}
+
+void bad_memchr(char *dest, const char *src) {
+ int length = strlen(src);
+ dest = (char *)memchr(src, '\0', length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: the length is too short for the last '\0' [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strchr(src, '\0');
+}
+
+void good_memchr(char *dest, const char *src) {
+ dest = strchr(src, '\0');
+}
+
+void bad_memmove(char *dest) {
+ memmove(dest, "foobar", strlen("foobar"));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memmove' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memmove_s(dest, strlen(dest), "foobar", strlen("foobar") + 1);
+}
+
+void good_memmove_safe(char *dest, const char *src) {
+ memmove_s(dest, strlen(dest), "foobar", strlen("foobar") + 1);
+}
+
+void bad_memmove_s(char *dest, const char *src) {
+ int length = strlen(src);
+ memmove_s(dest, strlen(dest), src, length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memmove_s' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memmove_s(dest, strlen(dest), src, length + 1);
+}
+
+void good_memmove_s_1(char *dest, const char *src) {
+ int length = strlen(src);
+ memmove_s(dest, strlen(dest), src, length + 1);
+}
+
+void good_memmove_s_2(char *dest, const char *src) {
+ int length = strlen(src);
+ memmove_s(dest, strlen(dest), (src + 1), length);
+}
+
+void bad_memset_1(char *dest, const char *src) {
+ int length = getLengthWithInc(src);
+ memset(dest, '-', length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memset(dest, '-', length - 1);
+}
+
+void good_memset1(char *dest, const char *src) {
+ int length = getLengthWithInc(src);
+ memset(dest, '-', length - 1);
+}
+
+void bad_memset_2(char *dest, const char *src) {
+ int length = strlen(src);
+ memset(dest, '-', length + 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memset(dest, '-', length);
+}
+
+void good_memset_2(char *dest, const char *src) {
+ int length = strlen(src);
+ memset(dest, '-', length);
+}
+
+void bad_strerror_s(char *dest, int errno) {
+ int length = strlen(strerror(errno));
+ strerror_s(dest, length, errno);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'strerror_s' is not null-terminated and missing the last character of the error message [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strerror_s(dest, length + 1, errno);
+}
+
+void good_strerror_s(char *dest, int errno) {
+ int length = strlen(strerror(errno));
+ strerror_s(dest, length + 1, errno);
+}
+
+int bad_strncmp_1(char *str1, const char *str2) {
+ int length = strlen(str1) + 1;
+ return strncmp(str1, str2, length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncmp(str1, str2, length - 1);
+}
+
+int good_strncmp_1(char *str1, const char *str2) {
+ int length = strlen(str1) + 1;
+ return strncmp(str1, str2, length - 1);
+}
+
+int bad_strncmp_2(char *str) {
+ return strncmp(str, "foobar", strlen("foobar") + 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncmp(str, "foobar", strlen("foobar"));
+}
+
+int bad_strncmp_3(char *str) {
+ return strncmp(str, "foobar", 1 + strlen("foobar"));
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strncmp(str, "foobar", strlen("foobar"));
+}
+
+int good_strncmp_2_3(char *str) {
+ return strncmp(str, "foobar", strlen("foobar"));
+}
+
+void bad_strxfrm(char *long_destination_name, const char *long_source_name) {
+ int very_long_length_definition_name = strlen(long_source_name);
+ strxfrm(long_destination_name, long_source_name,
+ very_long_length_definition_name);
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: the result from calling 'strxfrm' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: strxfrm(long_destination_name, long_source_name,
+ // CHECK-FIXES-NEXT: very_long_length_definition_name + 1);
+}
+
+void good_strxfrm(char *long_destination_name, const char *long_source_name) {
+ int very_long_length_definition_name = strlen(long_source_name);
+ strxfrm(long_destination_name, long_source_name,
+ very_long_length_definition_name + 1);
+}
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -36,6 +36,7 @@
bugprone-misplaced-widening-cast
bugprone-move-forwarding-reference
bugprone-multiple-statement-macro
+ bugprone-not-null-terminated-result
bugprone-parent-virtual-call
bugprone-sizeof-container
bugprone-sizeof-expression
Index: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
@@ -0,0 +1,108 @@
+.. title:: clang-tidy - bugprone-not-null-terminated-result
+
+bugprone-not-null-terminated-result
+===================================
+
+Finds function calls where ``strlen`` or ``wcslen`` function is passed as an
+argument and caused a not null-terminated result. Depending on the case of
+use, it may insert or remove the increase operation to ensure the result is
+null-terminated.
+
+The following function calls are checked:
+
+``alloca``, ``calloc``, ``malloc``, ``realloc``, ``memcpy``, ``wmemcpy``,
+``memcpy_s``, ``wmemcpy_s``, ``memchr``, ``wmemchr``, ``memmove``, ``wmemmove``,
+``memmove_s``, ``wmemmove_s``, ``memset``, ``wmemset``, ``strerror_s``,
+``strncmp``, ``wcsncmp``, ``strxfrm``, ``wcsxfrm``
+
+The following is a real-world example where the programmer forgot to increase
+the passed third argument, which is ``size_t length``. The proper length is
+``strlen(src) + 1`` because the null terminator need an extra space. That means
+the result is not null-terminated, which can result undefined behaviour when the
+string is read.
+
+.. code-block:: c
+
+ void bad_memcpy(char *dest, const char *src) {
+ memcpy(dest, src, strlen(src));
+ }
+
+In addition to issuing warnings, fix-it rewrites all the necessary code
+according to the given option. If the target version implements ``_s`` suffixed
+functions fix-it will rewrite it to the most safe function:
+
+.. code-block:: c
+
+ void good_memcpy_with_safer_functions(char *dest, const char *src) {
+ strncpy_s(dest, src, strlen(src));
+ }
+
+Otherwise fix-it will rewrite it to a safer function, that existed before ``_s``
+suffixed functions:
+
+.. code-block:: c
+
+ void good_memcpy_before_safer_functions(char *dest, const char *src) {
+ strncpy(dest, src, (strlen(src) + 1));
+ }
+
+Transformations
+---------------
+
+In general, the following transformations are could happen:
+
+**Memory allocation functions**
+
+- ``alloca`` function's argument gets a ``+ 1`` operation.
+
+- ``calloc`` function's first argument gets a ``+ 1`` operation.
+
+- ``malloc`` function's argument gets a ``+ 1`` operation.
+
+- ``realloc`` function's second argument gets a ``+ 1`` operation.
+
+**Memory handler functions**
+
+- ``memcpy``, ``wmemcpy``:
+ - If ``_s`` functions are available: New function is
+ ``strncpy_s``/``wcsncpy_s``, where no ``+ 1`` needed.
+
+ - Else: New function is ``strncpy``, where ``+ 1`` needed.
+
+- ``memchr``, ``wmemchr``:
+ - Usually there is a C-style cast, and it is needed to be removed, because the
+ new function ``strchr``/``wcschr``'s return type is correct.
+ - Also the third argument is not needed.
+
+- ``memmove``, ``wmemmove``:
+ - If ``_s`` functions are available: New function is
+ ``memmove_s``/``wmemmove_s``, it has four arguments,
+ - the new second argument is the first argument's length, and
+ - the third argument will be moved as the fourth, where ``+ 1`` needed.
+
+ - Else: The third argument gets a ``+ 1`` operation.
+
+- ``memmove_s``/``wmemmove_s``:
+ - The function's fourth argument gets a ``+ 1`` operation.
+
+- ``memset``/``wmemset``:
+ - The function's third argument has to be truncated without the ``+ 1`` part.
+
+**Character functions**
+
+- ``strerror_s`` functions's second argument get a ``+ 1`` operation.
+
+- ``strncmp``/``wcsncmp``:
+ - If the third argument is the first or the second argument's ``length + 1``,
+ then it has to be truncated without the ``+ 1`` operation.
+
+- ``strxfrm``/``wcsxfrm`` function's third argument gets a ``+ 1`` operation.
+
+Options
+-------
+
+.. option:: AreSafeFunctionsAvailable
+
+ An integer non-zero value specifying if the target environment implements
+ ``_s`` suffixed memory and character handler functions which are safer than
+ older versions (e.g. ``memcpy_s()``). The default value is ``1``.
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -64,6 +64,14 @@
- New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
+- New :doc:`bugprone-not-null-terminated-result
+ <clang-tidy/checks/bugprone-not-null-terminated-result>` check
+
+ Finds function calls where ``strlen`` or ``wcslen`` function is passed as an
+ argument and caused a not null-terminated result. Depending on the case of
+ use, it may insert or remove the increase operation to ensure the result is
+ null-terminated.
+
- New :doc:`bugprone-parent-virtual-call
<clang-tidy/checks/bugprone-parent-virtual-call>` check
Index: clang-tidy/bugprone/NotNullTerminatedResultCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/bugprone/NotNullTerminatedResultCheck.h
@@ -0,0 +1,67 @@
+//===--------------- NotNullTerminatedResultCheck.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_BUGPRONE_NOT_NULL_TERMINATED_RESULT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOT_NULL_TERMINATED_RESULT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds function calls where ``strlen`` or ``wcslen`` function is passed as an
+/// argument and caused a not null-terminated result. Depending on the case of
+/// use, it may insert or remove the increase operation to ensure the result is
+/// null-terminated.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-not-null-terminated-result.html
+class NotNullTerminatedResultCheck : public ClangTidyCheck {
+public:
+ NotNullTerminatedResultCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ // If non-zero the target environment implements '_s' suffixed memory and
+ // character handler functions which are safer than older versions
+ // (e.g. 'memcpy_s()').
+ const int AreSafeFunctionsAvailable;
+
+ void memAllocFuncFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result);
+ void memHandlerFuncFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result);
+ void memcpyFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag);
+ void memcpy_sFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag);
+ void memchrFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result);
+ void memmoveFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag);
+ void characterFuncFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result);
+ void strerror_sFix(const ast_matchers::MatchFinder::MatchResult &Result);
+ void ncmpFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result);
+ void xfrmFix(StringRef Name,
+ const ast_matchers::MatchFinder::MatchResult &Result);
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOT_NULL_TERMINATED_RESULT_H
Index: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
@@ -0,0 +1,494 @@
+//===------------- NotNullTerminatedResultCheck.cpp - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NotNullTerminatedResultCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+static const char *const FuncExprName = "entire-called-function-expr";
+static const char *const CastExprName = "cast-expr";
+static const char *const LengthCallName = "simple-strlen-or-wcslen-call";
+
+static void lengthArgRemoveIncByOne(int ArgPos,
+ const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag);
+static void lengthArgInsertIncByOne(int ArgPos,
+ const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag);
+static void removeArg(int ArgPos, const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag);
+static void renameFunc(StringRef NewFuncName,
+ const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag);
+static StringRef exprToStr(const Expr *E,
+ const MatchFinder::MatchResult &Result);
+static SourceLocation exprLocEnd(const Expr *E,
+ const MatchFinder::MatchResult &Result);
+static bool isTrimMemmove(const MatchFinder::MatchResult &Result);
+
+NotNullTerminatedResultCheck::NotNullTerminatedResultCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ AreSafeFunctionsAvailable(Options.get("AreSafeFunctionsAvailable", 1)) {}
+
+void NotNullTerminatedResultCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "AreSafeFunctionsAvailable", AreSafeFunctionsAvailable);
+}
+
+void NotNullTerminatedResultCheck::registerMatchers(MatchFinder *Finder) {
+ const auto LengthCall =
+ callExpr(callee(functionDecl(hasAnyName("::strlen", "::wcslen"))))
+ .bind(LengthCallName);
+ const auto HasIntOperand =
+ hasEitherOperand(ignoringParenImpCasts(integerLiteral()));
+ const auto HasInc = binaryOperator(hasOperatorName("+"), HasIntOperand);
+
+ // The following seven case match 'strlen' or 'wcslen' function calls where no
+ // increase by one operation found.
+ // - Example: strlen(src) or wcslen(src)
+ const auto SimpleCallWithoutInc = LengthCall;
+
+ // - Example: (strlen(src) * 2) or (wcslen(src) * 2)
+ const auto ComplexCallWithoutInc =
+ binaryOperator(unless(hasOperatorName("+")),
+ hasEitherOperand(ignoringImpCasts(LengthCall)));
+
+ // - Example: (strlen(dest) + strlen(src)) or (wcslen(dest) + wcslen(src))
+ const auto ComplexTwoCallWithoutInc =
+ binaryOperator(hasOperatorName("+"), unless(HasIntOperand),
+ hasLHS(ignoringImpCasts(LengthCall)),
+ hasRHS(ignoringImpCasts(LengthCall)));
+
+ const auto AnyOfCallWithoutInc = ignoringImpCasts(anyOf(
+ SimpleCallWithoutInc, ComplexCallWithoutInc, ComplexTwoCallWithoutInc));
+
+ // - Example: length = strlen(src);
+ const auto DREWithoutInc = ignoringImpCasts(
+ declRefExpr(to(varDecl(hasInitializer(AnyOfCallWithoutInc)))));
+
+ // - Example: malloc(length * 2)
+ const auto OpHasDREWithoutInc = binaryOperator(
+ unless(hasOperatorName("+")), hasEitherOperand(DREWithoutInc));
+
+ const auto AnyOfCallOrDREWithoutInc =
+ anyOf(OpHasDREWithoutInc, DREWithoutInc, AnyOfCallWithoutInc);
+
+ // - Example: int getLength(const char *str) { return strlen(str); }
+ const auto CallExprReturnWithoutInc =
+ ignoringImpCasts(callExpr(callee(functionDecl(hasBody(
+ has(returnStmt(hasReturnValue(AnyOfCallOrDREWithoutInc))))))));
+
+ // - Example: malloc(getLength(str) * 2)
+ const auto CallExprOpHasReturnWithoutInc = binaryOperator(
+ unless(hasOperatorName("+")), hasEitherOperand(CallExprReturnWithoutInc));
+
+ const auto AnyOfCallExprReturnWithoutInc =
+ anyOf(CallExprReturnWithoutInc, CallExprOpHasReturnWithoutInc);
+
+ // - Example: int length = getLength(src);
+ const auto DREHasReturnWithoutInc = ignoringImpCasts(
+ declRefExpr(to(varDecl(hasInitializer(AnyOfCallExprReturnWithoutInc)))));
+
+ const auto LengthWithoutInc =
+ anyOf(AnyOfCallOrDREWithoutInc, AnyOfCallExprReturnWithoutInc,
+ DREHasReturnWithoutInc);
+
+ // The following four case match 'strlen' or 'wcslen' function calls where
+ // increase by one operation found.
+ // - Example: (strlen(src) + 1) or (wcslen(src) + 1)
+ const auto SimpleCallWithInc = allOf(
+ HasInc, binaryOperator(hasEitherOperand(ignoringImpCasts(LengthCall))));
+
+ // - Example: ((strlen(src) / 2) + 1) or ((wcslen(src) / 2) + 1)
+ const auto ComplexCallWithInc =
+ allOf(HasInc, binaryOperator(has(binaryOperator(
+ unless(hasOperatorName("+")),
+ hasEitherOperand(ignoringImpCasts(LengthCall))))));
+
+ // - Example: int length = strlen(src); memcpy(dest, src, length + 1)
+ const auto OpHasDREWithInc =
+ binaryOperator(hasEitherOperand(ignoringImpCasts(DREWithoutInc)));
+
+ const auto AnyOfCallWithInc = anyOf(SimpleCallWithInc, ComplexCallWithInc);
+
+ const auto AnyOfCallOrDREWithInc = ignoringImpCasts(
+ anyOf(OpHasDREWithInc, declRefExpr(to(varDecl(hasInitializer(
+ ignoringImpCasts(AnyOfCallWithInc))))),
+ AnyOfCallWithInc));
+
+ // - Example: int getLength(const char *str) { return strlen(str) + 1; }
+ const auto CallExprReturnWithInc =
+ ignoringImpCasts(callExpr(callee(functionDecl(
+ hasBody(has(returnStmt(hasReturnValue(AnyOfCallOrDREWithInc))))))));
+
+ // - Example: int length = getLength(src);
+ const auto DREHasReturnWithInc = ignoringImpCasts(
+ declRefExpr(to(varDecl(hasInitializer(CallExprReturnWithInc)))));
+
+ const auto LengthWithInc =
+ anyOf(AnyOfCallOrDREWithInc, CallExprReturnWithInc, DREHasReturnWithInc);
+
+ enum StrLenKind { WithInc, WithoutInc };
+
+ const auto Matcher = [=](StringRef Name, int ArgCount, int ArgPos,
+ StrLenKind Kind) {
+ if (Kind == StrLenKind::WithoutInc) {
+ return allOf(callee(functionDecl(hasName(Name))),
+ argumentCountIs(ArgCount),
+ hasArgument(ArgPos, ignoringImpCasts(LengthWithoutInc)));
+ } else {
+ return allOf(callee(functionDecl(hasName(Name))),
+ argumentCountIs(ArgCount),
+ hasArgument(ArgPos, ignoringImpCasts(LengthWithInc)));
+ }
+ };
+
+ const auto Alloca = Matcher("::alloca", 1, 0, StrLenKind::WithoutInc);
+ const auto Calloc = Matcher("::calloc", 2, 0, StrLenKind::WithoutInc);
+ const auto Malloc = Matcher("::malloc", 1, 0, StrLenKind::WithoutInc);
+ const auto Realloc = Matcher("::realloc", 2, 1, StrLenKind::WithoutInc);
+ const auto Memcpy = Matcher("::memcpy", 3, 2, StrLenKind::WithoutInc);
+ const auto Wmemcpy = Matcher("::wmemcpy", 3, 2, StrLenKind::WithoutInc);
+ const auto Memcpy_s = Matcher("::memcpy_s", 4, 3, StrLenKind::WithoutInc);
+ const auto Wmemcpy_s = Matcher("::wmemcpy_s", 4, 3, StrLenKind::WithoutInc);
+ const auto Memchr = Matcher("::memchr", 3, 2, StrLenKind::WithoutInc);
+ const auto Wmemchr = Matcher("::wmemchr", 3, 2, StrLenKind::WithoutInc);
+ const auto Memmove = Matcher("::memmove", 3, 2, StrLenKind::WithoutInc);
+ const auto Wmemmove = Matcher("::wmemmove", 3, 2, StrLenKind::WithoutInc);
+ const auto Memmove_s = Matcher("::memmove_s", 4, 3, StrLenKind::WithoutInc);
+ const auto Wmemmove_s = Matcher("::wmemmove_s", 4, 3, StrLenKind::WithoutInc);
+ const auto Memset = Matcher("::memset", 3, 2, StrLenKind::WithInc);
+ const auto Wmemset = Matcher("::wmemset", 3, 2, StrLenKind::WithInc);
+ const auto Strerror_s = Matcher("::strerror_s", 3, 1, StrLenKind::WithoutInc);
+ const auto Strncmp = Matcher("::strncmp", 3, 2, StrLenKind::WithInc);
+ const auto Wcsncmp = Matcher("::wcsncmp", 3, 2, StrLenKind::WithInc);
+ const auto Strxfrm = Matcher("::strxfrm", 3, 2, StrLenKind::WithoutInc);
+ const auto Wcsxfrm = Matcher("::wcsxfrm", 3, 2, StrLenKind::WithoutInc);
+
+ const auto AnyOfExpressions = anyOf(
+ Alloca, Calloc, Malloc, Realloc, Memcpy, Wmemcpy, Memcpy_s, Wmemcpy_s,
+ Memchr, Wmemchr, Memmove, Wmemmove, Memmove_s, Wmemmove_s, Memset,
+ Wmemset, Strerror_s, Strncmp, Wcsncmp, Strxfrm, Wcsxfrm);
+
+ Finder->addMatcher(callExpr(AnyOfExpressions).bind(FuncExprName), this);
+ Finder->addMatcher(
+ binaryOperator(
+ has(cStyleCastExpr(
+ has(callExpr(anyOf(Memchr, Wmemchr)).bind(FuncExprName)))
+ .bind(CastExprName))),
+ this);
+}
+
+void NotNullTerminatedResultCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+
+ if (FuncExpr->getLocStart().isMacroID())
+ return;
+
+ if (!FuncExpr->getDirectCallee())
+ return;
+
+ StringRef Name = FuncExpr->getDirectCallee()->getName();
+ if (Name.contains("alloc"))
+ memAllocFuncFix(Name, Result);
+ else if (Name.startswith("mem") || Name.startswith("wmem"))
+ memHandlerFuncFix(Name, Result);
+ else if (Name.startswith("str") || Name.startswith("wcs"))
+ characterFuncFix(Name, Result);
+}
+
+void NotNullTerminatedResultCheck::memAllocFuncFix(
+ StringRef Name, const MatchFinder::MatchResult &Result) {
+ auto Diag =
+ diag(Result.Nodes.getNodeAs<CallExpr>(FuncExprName)->getLocStart(),
+ "memory allocated by '%0' is insufficient to hold the null "
+ "terminator")
+ << Name;
+
+ if (Name == "realloc")
+ lengthArgInsertIncByOne(1, Result, Diag);
+ else
+ lengthArgInsertIncByOne(0, Result, Diag);
+}
+
+void NotNullTerminatedResultCheck::memHandlerFuncFix(
+ StringRef Name, const MatchFinder::MatchResult &Result) {
+ if (Name.contains("memmove") && isTrimMemmove(Result))
+ return;
+
+ if (Name.endswith("chr")) {
+ memchrFix(Name, Result);
+ return;
+ }
+
+ auto Diag =
+ diag(Result.Nodes.getNodeAs<CallExpr>(FuncExprName)->getLocStart(),
+ "the result from calling '%0' is not null-terminated")
+ << Name;
+
+ if (Name.endswith("cpy"))
+ memcpyFix(Name, Result, Diag);
+ else if (Name.endswith("cpy_s"))
+ memcpy_sFix(Name, Result, Diag);
+ else if (Name.endswith("move"))
+ memmoveFix(Name, Result, Diag);
+ else if (Name.endswith("move_s"))
+ lengthArgInsertIncByOne(3, Result, Diag);
+ else if (Name.endswith("set"))
+ lengthArgRemoveIncByOne(2, Result, Diag);
+}
+
+void NotNullTerminatedResultCheck::memcpyFix(
+ StringRef Name, const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag) {
+ if (AreSafeFunctionsAvailable) {
+ StringRef NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s";
+ renameFunc(NewFuncName, Result, Diag);
+ } else {
+ StringRef NewFuncName = (Name[0] != 'w') ? "strncpy" : "wcsncpy";
+ renameFunc(NewFuncName, Result, Diag);
+ lengthArgInsertIncByOne(2, Result, Diag);
+ }
+}
+
+void NotNullTerminatedResultCheck::memcpy_sFix(
+ StringRef Name, const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag) {
+ StringRef NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s";
+ renameFunc(NewFuncName, Result, Diag);
+ removeArg(1, Result, Diag);
+}
+
+void NotNullTerminatedResultCheck::memchrFix(
+ StringRef Name, const MatchFinder::MatchResult &Result) {
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+ if (exprToStr(FuncExpr->getArg(1), Result) != "\'\\0\'")
+ return;
+
+ auto Diag = diag(FuncExpr->getArg(2)->IgnoreParenCasts()->getLocStart(),
+ "the length is too short for the last \'\\0\'");
+
+ if (const auto *CastExpr =
+ Result.Nodes.getNodeAs<CStyleCastExpr>(CastExprName)) {
+ const auto CastRemoveFix = FixItHint::CreateRemoval(SourceRange(
+ CastExpr->getLocStart(), FuncExpr->getLocStart().getLocWithOffset(-1)));
+ Diag << CastRemoveFix;
+ }
+ StringRef NewFuncName = (Name[0] != 'w') ? "strchr" : "wcschr";
+ renameFunc(NewFuncName, Result, Diag);
+ removeArg(2, Result, Diag);
+}
+
+void NotNullTerminatedResultCheck::memmoveFix(
+ StringRef Name, const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag) {
+ if (AreSafeFunctionsAvailable) {
+ StringRef NewFuncName = (Name[0] != 'w') ? "memmove_s" : "wmemmove_s";
+ renameFunc(NewFuncName, Result, Diag);
+
+ const auto FirstArg =
+ Result.Nodes.getNodeAs<CallExpr>(FuncExprName)->getArg(0);
+ SmallString<64> NewSecondArg;
+ NewSecondArg += " strlen(";
+ NewSecondArg += exprToStr(FirstArg, Result);
+ NewSecondArg += "),";
+
+ const auto InsertNewArgFix = FixItHint::CreateInsertion(
+ exprLocEnd(FirstArg, Result).getLocWithOffset(1), NewSecondArg);
+ Diag << InsertNewArgFix;
+ }
+ lengthArgInsertIncByOne(2, Result, Diag);
+}
+
+void NotNullTerminatedResultCheck::characterFuncFix(
+ StringRef Name, const MatchFinder::MatchResult &Result) {
+ if (Name == "strerror_s")
+ strerror_sFix(Result);
+ else if (Name.endswith("ncmp"))
+ ncmpFix(Name, Result);
+ else if (Name.endswith("xfrm"))
+ xfrmFix(Name, Result);
+}
+
+void NotNullTerminatedResultCheck::strerror_sFix(
+ const MatchFinder::MatchResult &Result) {
+ auto Diag =
+ diag(Result.Nodes.getNodeAs<CallExpr>(FuncExprName)->getLocStart(),
+ "the result from calling 'strerror_s' is not null-terminated and "
+ "missing the last character of the error message");
+ lengthArgInsertIncByOne(1, Result, Diag);
+}
+
+void NotNullTerminatedResultCheck::ncmpFix(
+ StringRef Name, const MatchFinder::MatchResult &Result) {
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+ const auto *LengthExprArg =
+ Result.Nodes.getNodeAs<CallExpr>(LengthCallName)->getArg(0);
+ StringRef FirstArgStr = exprToStr(FuncExpr->getArg(0), Result);
+ StringRef SecondArgStr = exprToStr(FuncExpr->getArg(1), Result);
+ StringRef LengthExprArgStr = exprToStr(LengthExprArg, Result);
+
+ if (LengthExprArgStr == FirstArgStr || LengthExprArgStr == SecondArgStr) {
+ auto Diag = diag(
+ FuncExpr->getArg(2)->IgnoreParenCasts()->getLocStart(),
+ "comparison length is too long and might lead to a buffer overflow");
+ lengthArgRemoveIncByOne(2, Result, Diag);
+ }
+}
+
+void NotNullTerminatedResultCheck::xfrmFix(
+ StringRef Name, const MatchFinder::MatchResult &Result) {
+ auto Diag =
+ diag(Result.Nodes.getNodeAs<CallExpr>(FuncExprName)->getLocStart(),
+ "the result from calling '%0' is not null-terminated")
+ << Name;
+ lengthArgInsertIncByOne(2, Result, Diag);
+}
+
+static void lengthArgInsertIncByOne(int ArgPos,
+ const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag) {
+ const auto *LengthExpr =
+ Result.Nodes.getNodeAs<CallExpr>(FuncExprName)->getArg(ArgPos);
+ const bool NeedInnerParen =
+ isa<BinaryOperator>(LengthExpr) &&
+ dyn_cast<BinaryOperator>(LengthExpr)->getOpcode() != BO_Add;
+
+ if (NeedInnerParen) {
+ const auto InsertFirstParenFix =
+ FixItHint::CreateInsertion(LengthExpr->getLocStart(), "(");
+ const auto InsertPlusOneAndSecondParenFix =
+ FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), ") + 1");
+ Diag << InsertFirstParenFix << InsertPlusOneAndSecondParenFix;
+ } else {
+ const auto InsertPlusOne =
+ FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), " + 1");
+ Diag << InsertPlusOne;
+ }
+}
+
+static void lengthArgRemoveIncByOne(int ArgPos,
+ const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag) {
+ const auto *LengthExpr =
+ Result.Nodes.getNodeAs<CallExpr>(FuncExprName)->getArg(ArgPos);
+
+ // This is the following structure: ((strlen(src) * 2) + 1)
+ // InnerOpExpr: ~~~~~~~~~~~~^~~
+ // OuterOpExpr ~~~~~~~~~~~~~~~~~~^~~
+ if (const auto *OuterOpExpr =
+ dyn_cast<BinaryOperator>(LengthExpr->IgnoreParenCasts())) {
+ const auto *LHSExpr = OuterOpExpr->getLHS();
+ const auto *RHSExpr = OuterOpExpr->getRHS();
+ const auto *InnerOpExpr =
+ isa<IntegerLiteral>(RHSExpr->IgnoreCasts()) ? LHSExpr : RHSExpr;
+
+ // This is the following structure: ((strlen(src) * 2) + 1)
+ // LHSRemoveRange: ~
+ // RHSRemoveRange: ~~~~~
+ const auto LHSRemoveRange =
+ SourceRange(LengthExpr->getLocStart(),
+ InnerOpExpr->getLocStart().getLocWithOffset(-1));
+ const auto RHSRemoveRange = SourceRange(
+ InnerOpExpr->getLocEnd().getLocWithOffset(1), LengthExpr->getLocEnd());
+
+ const auto LHSRemoveFix = FixItHint::CreateRemoval(LHSRemoveRange);
+ const auto RHSRemoveFix = FixItHint::CreateRemoval(RHSRemoveRange);
+ if (LengthExpr->getLocStart() == InnerOpExpr->getLocStart())
+ Diag << RHSRemoveFix;
+ else if (LengthExpr->getLocEnd() == InnerOpExpr->getLocEnd())
+ Diag << LHSRemoveFix;
+ else
+ Diag << LHSRemoveFix << RHSRemoveFix;
+ } else {
+ const auto InsertMinusOneFix =
+ FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), " - 1");
+ Diag << InsertMinusOneFix;
+ }
+}
+
+static void removeArg(int ArgPos, const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag) {
+ // This is the following structure: (src, '\0', strlen(src))
+ // ArgToRemove: ~~~~~~~~~~~
+ // LHSArg: ~~~~
+ // RemoveArgFix: ~~~~~~~~~~~~~
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+ const auto ArgToRemove = FuncExpr->getArg(ArgPos);
+ const auto LHSArg = FuncExpr->getArg(ArgPos - 1);
+ const auto RemoveArgFix = FixItHint::CreateRemoval(
+ SourceRange(exprLocEnd(LHSArg, Result),
+ exprLocEnd(ArgToRemove, Result).getLocWithOffset(-1)));
+ Diag << RemoveArgFix;
+}
+
+static void renameFunc(StringRef NewFuncName,
+ const MatchFinder::MatchResult &Result,
+ DiagnosticBuilder &Diag) {
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+ const auto FuncNameLength =
+ FuncExpr->getDirectCallee()->getIdentifier()->getLength();
+ const auto FirstArgLoc = FuncExpr->getArg(0)->getExprLoc();
+ const auto FuncNameRange = CharSourceRange::getCharRange(
+ FirstArgLoc.getLocWithOffset(-FuncNameLength - 1),
+ FirstArgLoc.getLocWithOffset(-1));
+
+ const auto FuncNameFix =
+ FixItHint::CreateReplacement(FuncNameRange, NewFuncName);
+ Diag << FuncNameFix;
+}
+
+static StringRef exprToStr(const Expr *E,
+ const MatchFinder::MatchResult &Result) {
+ return Lexer::getSourceText(
+ CharSourceRange::getTokenRange(E->getSourceRange()),
+ *Result.SourceManager, Result.Context->getLangOpts(), 0);
+}
+
+static SourceLocation exprLocEnd(const Expr *E,
+ const MatchFinder::MatchResult &Result) {
+ return Lexer::getLocForEndOfToken(E->getLocEnd(), 0, *Result.SourceManager,
+ Result.Context->getLangOpts());
+}
+
+/// Sometimes the 'memmove' function is in use to trim the text until it matches
+/// to a specific condition. For example a .CSV file's line would be trimmed
+/// from ' "data_name", "data", ' to just 'data' by "memmoving" it char-by-char.
+static bool isTrimMemmove(const MatchFinder::MatchResult &Result) {
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+ const int ArgPos = FuncExpr->getNumArgs() - 2;
+
+ // This is the following structure: (dest, (src + 1), strlen(src))
+ // LHSStr: ~~~
+ // RHSStr: ~
+ // LengthExprArgStr: ~~~
+ if (const auto *BinOp = dyn_cast<BinaryOperator>(
+ FuncExpr->getArg(ArgPos)->IgnoreParenCasts())) {
+ StringRef LHSStr = exprToStr(BinOp->getLHS()->IgnoreParens(), Result);
+ StringRef RHSStr = exprToStr(BinOp->getRHS()->IgnoreParens(), Result);
+ StringRef LengthExprArgStr = exprToStr(
+ Result.Nodes.getNodeAs<CallExpr>(LengthCallName)->getArg(0), Result);
+
+ return LengthExprArgStr == LHSStr || LengthExprArgStr == RHSStr;
+ }
+ return false;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tidy/bugprone/CMakeLists.txt
+++ clang-tidy/bugprone/CMakeLists.txt
@@ -20,6 +20,7 @@
MisplacedWideningCastCheck.cpp
MoveForwardingReferenceCheck.cpp
MultipleStatementMacroCheck.cpp
+ NotNullTerminatedResultCheck.cpp
ParentVirtualCallCheck.cpp
SizeofContainerCheck.cpp
SizeofExpressionCheck.cpp
Index: clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -28,6 +28,7 @@
#include "MisplacedWideningCastCheck.h"
#include "MoveForwardingReferenceCheck.h"
#include "MultipleStatementMacroCheck.h"
+#include "NotNullTerminatedResultCheck.h"
#include "ParentVirtualCallCheck.h"
#include "SizeofContainerCheck.h"
#include "SizeofExpressionCheck.h"
@@ -91,6 +92,8 @@
"bugprone-move-forwarding-reference");
CheckFactories.registerCheck<MultipleStatementMacroCheck>(
"bugprone-multiple-statement-macro");
+ CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
+ "bugprone-not-null-terminated-result");
CheckFactories.registerCheck<ParentVirtualCallCheck>(
"bugprone-parent-virtual-call");
CheckFactories.registerCheck<SizeofContainerCheck>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
