Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r14-3740-gb51cde34d4e750.
gcc/analyzer/ChangeLog: PR analyzer/105899 * kf.cc (class kf_strncpy): New. (kf_strncpy::impl_call_post): New. (register_known_functions): Register it. * region-model.cc (region_model::read_bytes): Handle unknown number of bytes. gcc/testsuite/ChangeLog: PR analyzer/105899 * c-c++-common/analyzer/null-terminated-strings-2.c: New test. * c-c++-common/analyzer/overlapping-buffers.c: Update dg-bogus directives to avoid clashing with note from <string.h> that might happen to have the same line number. Add strpncpy test coverage. * c-c++-common/analyzer/strncpy-1.c: New test. * gcc.dg/analyzer/null-terminated-strings-1.c (test_filled_nonzero): New. (void test_filled_zero): New. (test_filled_symbolic): New. --- gcc/analyzer/kf.cc | 182 ++++++++++++++++++ gcc/analyzer/region-model.cc | 2 + .../analyzer/null-terminated-strings-2.c | 17 ++ .../analyzer/overlapping-buffers.c | 24 ++- .../c-c++-common/analyzer/strncpy-1.c | 157 +++++++++++++++ .../analyzer/null-terminated-strings-1.c | 24 +++ 6 files changed, 398 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/analyzer/null-terminated-strings-2.c create mode 100644 gcc/testsuite/c-c++-common/analyzer/strncpy-1.c diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc index a62227729991..8a45c329c282 100644 --- a/gcc/analyzer/kf.cc +++ b/gcc/analyzer/kf.cc @@ -1375,6 +1375,186 @@ make_kf_strlen () return make_unique<kf_strlen> (); } +/* Handler for "strncpy" and "__builtin_strncpy". + See e.g. https://en.cppreference.com/w/c/string/byte/strncpy + + extern char *strncpy (char *dst, const char *src, size_t count); + + Handle this by splitting into two outcomes: + (a) truncated read from "src" of "count" bytes, + writing "count" bytes to "dst" + (b) read from "src" of up to (and including) the null terminator, + where the number of bytes read < "count" bytes, + writing those bytes to "dst", and zero-filling the rest, + up to "count". */ + +class kf_strncpy : public builtin_known_function +{ +public: + bool matches_call_types_p (const call_details &cd) const final override + { + return (cd.num_args () == 3 + && cd.arg_is_pointer_p (0) + && cd.arg_is_pointer_p (1) + && cd.arg_is_integral_p (2)); + } + enum built_in_function builtin_code () const final override + { + return BUILT_IN_STRNCPY; + } + void impl_call_post (const call_details &cd) const final override; +}; + +void +kf_strncpy::impl_call_post (const call_details &cd) const +{ + class strncpy_call_info : public call_info + { + public: + strncpy_call_info (const call_details &cd, + const svalue *num_bytes_with_terminator_sval, + bool truncated_read) + : call_info (cd), + m_num_bytes_with_terminator_sval (num_bytes_with_terminator_sval), + m_truncated_read (truncated_read) + { + } + + label_text get_desc (bool can_colorize) const final override + { + if (m_truncated_read) + return make_label_text (can_colorize, + "when %qE truncates the source string", + get_fndecl ()); + else + return make_label_text (can_colorize, + "when %qE copies the full source string", + get_fndecl ()); + } + + bool update_model (region_model *model, + const exploded_edge *, + region_model_context *ctxt) const final override + { + const call_details cd (get_call_details (model, ctxt)); + + const svalue *dest_sval = cd.get_arg_svalue (0); + const region *dest_reg + = model->deref_rvalue (dest_sval, cd.get_arg_tree (0), ctxt); + + const svalue *src_sval = cd.get_arg_svalue (1); + const region *src_reg + = model->deref_rvalue (src_sval, cd.get_arg_tree (1), ctxt); + + const svalue *count_sval = cd.get_arg_svalue (2); + + /* strncpy returns the initial param. */ + cd.maybe_set_lhs (dest_sval); + + const svalue *num_bytes_read_sval; + if (m_truncated_read) + { + /* Truncated read. */ + num_bytes_read_sval = count_sval; + + if (m_num_bytes_with_terminator_sval) + { + /* The terminator is after the limit. */ + if (!model->add_constraint (m_num_bytes_with_terminator_sval, + GT_EXPR, + count_sval, + ctxt)) + return false; + } + else + { + /* We don't know where the terminator is, or if there is one. + In theory we know that the first COUNT bytes are non-zero, + but we don't have a way to record that constraint. */ + } + } + else + { + /* Full read of the src string before reaching the limit, + so there must be a terminator and it must be at or before + the limit. */ + if (m_num_bytes_with_terminator_sval) + { + if (!model->add_constraint (m_num_bytes_with_terminator_sval, + LE_EXPR, + count_sval, + ctxt)) + return false; + num_bytes_read_sval = m_num_bytes_with_terminator_sval; + + /* First, zero-fill the dest buffer. + We don't need to do this for the truncation case, as + this fully populates the dest buffer. */ + const region *sized_dest_reg + = model->get_manager ()->get_sized_region (dest_reg, + NULL_TREE, + count_sval); + model->zero_fill_region (sized_dest_reg, ctxt); + } + else + { + /* Don't analyze this case; the other case will + assume a "truncated" read up to the limit. */ + return false; + } + } + + gcc_assert (num_bytes_read_sval); + + const svalue *bytes_to_copy + = model->read_bytes (src_reg, + cd.get_arg_tree (1), + num_bytes_read_sval, + ctxt); + cd.complain_about_overlap (0, 1, num_bytes_read_sval); + model->write_bytes (dest_reg, + num_bytes_read_sval, + bytes_to_copy, + ctxt); + + return true; + } + private: + /* (strlen + 1) of the source string if it has a terminator, + or NULL for the case where UB would happen before + finding any terminator. */ + const svalue *m_num_bytes_with_terminator_sval; + + /* true: if this is the outcome where the limit was reached before + the null terminator + false: if the null terminator was reached before the limit. */ + bool m_truncated_read; + }; + + /* Body of kf_strncpy::impl_call_post. */ + if (cd.get_ctxt ()) + { + /* First, scan for a null terminator as if there were no limit, + with a null ctxt so no errors are reported. */ + const region_model *model = cd.get_model (); + const svalue *ptr_arg_sval = cd.get_arg_svalue (1); + const region *buf_reg + = model->deref_rvalue (ptr_arg_sval, cd.get_arg_tree (1), nullptr); + const svalue *num_bytes_with_terminator_sval + = model->scan_for_null_terminator (buf_reg, + cd.get_arg_tree (1), + nullptr, + nullptr); + cd.get_ctxt ()->bifurcate + (make_unique<strncpy_call_info> (cd, num_bytes_with_terminator_sval, + false)); + cd.get_ctxt ()->bifurcate + (make_unique<strncpy_call_info> (cd, num_bytes_with_terminator_sval, + true)); + cd.get_ctxt ()->terminate_path (); + } +}; + /* Handler for "strndup" and "__builtin_strndup". */ class kf_strndup : public builtin_known_function @@ -1620,6 +1800,8 @@ register_known_functions (known_function_manager &kfm) kfm.add ("__builtin___strcat_chk", make_unique<kf_strcat> (3, true)); kfm.add ("strdup", make_unique<kf_strdup> ()); kfm.add ("__builtin_strdup", make_unique<kf_strdup> ()); + kfm.add ("strncpy", make_unique<kf_strncpy> ()); + kfm.add ("__builtin_strncpy", make_unique<kf_strncpy> ()); kfm.add ("strndup", make_unique<kf_strndup> ()); kfm.add ("__builtin_strndup", make_unique<kf_strndup> ()); kfm.add ("strlen", make_unique<kf_strlen> ()); diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 6be0ad72aaae..999480e55ef7 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -3979,6 +3979,8 @@ region_model::read_bytes (const region *src_reg, const svalue *num_bytes_sval, region_model_context *ctxt) const { + if (num_bytes_sval->get_kind () == SK_UNKNOWN) + return m_mgr->get_or_create_unknown_svalue (NULL_TREE); const region *sized_src_reg = m_mgr->get_sized_region (src_reg, NULL_TREE, num_bytes_sval); const svalue *src_contents_sval = get_store_value (sized_src_reg, ctxt); diff --git a/gcc/testsuite/c-c++-common/analyzer/null-terminated-strings-2.c b/gcc/testsuite/c-c++-common/analyzer/null-terminated-strings-2.c new file mode 100644 index 000000000000..3bbae821cd46 --- /dev/null +++ b/gcc/testsuite/c-c++-common/analyzer/null-terminated-strings-2.c @@ -0,0 +1,17 @@ +#include "../../gcc.dg/analyzer/analyzer-decls.h" + +char buf[16]; + +int main (void) +{ + /* We should be able to assume that "buf" is all zeroes here. */ + + __analyzer_eval (__analyzer_get_strlen (buf) == 0); /* { dg-warning "TRUE" "ideal" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + + buf[0] = 'a'; + __analyzer_eval (__analyzer_get_strlen (buf) == 1); /* { dg-warning "TRUE" "ideal" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + + return 0; +} diff --git a/gcc/testsuite/c-c++-common/analyzer/overlapping-buffers.c b/gcc/testsuite/c-c++-common/analyzer/overlapping-buffers.c index 5808b3304cfc..f02238dfa42c 100644 --- a/gcc/testsuite/c-c++-common/analyzer/overlapping-buffers.c +++ b/gcc/testsuite/c-c++-common/analyzer/overlapping-buffers.c @@ -55,7 +55,7 @@ void test_memcpy_symbolic_1 (void *p, size_t n) void * __attribute__((noinline)) call_memcpy_nonintersecting_concrete_1 (void *dest, const void *src, size_t n) { - return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers" } */ + return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers passed as" } */ } void test_memcpy_nonintersecting_concrete_1 (char *p) @@ -66,7 +66,7 @@ void test_memcpy_nonintersecting_concrete_1 (char *p) void * __attribute__((noinline)) call_memcpy_nonintersecting_concrete_2 (void *dest, const void *src, size_t n) { - return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers" } */ + return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers passed as" } */ } void test_memcpy_nonintersecting_concrete_2 (char *p) @@ -111,7 +111,7 @@ void test_memcpy_intersecting_symbolic_1 (char *p, size_t n) void * __attribute__((noinline)) call_memcpy_nonintersecting_symbolic_1 (void *dest, const void *src, size_t n) { - return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers" } */ + return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers passed as" } */ } void test_memcpy_nonintersecting_symbolic_1 (char *p, size_t n) @@ -122,7 +122,7 @@ void test_memcpy_nonintersecting_symbolic_1 (char *p, size_t n) void * __attribute__((noinline)) call_memcpy_nonintersecting_symbolic_2 (void *dest, const void *src, size_t n) { - return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers" } */ + return memcpy (dest, src, n); /* { dg-bogus "overlapping buffers passed as" } */ } void test_memcpy_nonintersecting_symbolic_2 (char *p, size_t n) @@ -134,7 +134,7 @@ void test_memcpy_nonintersecting_symbolic_2 (char *p, size_t n) void * __attribute__((noinline)) call_memmove_symbolic_1 (void *dest, const void *src, size_t n) { - return memmove (dest, src, n); /* { dg-bogus "overlapping buffers" } */ + return memmove (dest, src, n); /* { dg-bogus "overlapping buffers passed as" } */ } void test_memmove_symbolic_1 (void *p, size_t n) @@ -142,6 +142,14 @@ void test_memmove_symbolic_1 (void *p, size_t n) call_memmove_symbolic_1 (p, p, n); } -/* TODO: - - strncpy - */ +static char * __attribute__((noinline)) +call_strncpy_1 (char *dest, const char *src, size_t n) +{ + return strncpy (dest, src, n); /* { dg-warning "overlapping buffers" } */ +} + +void +test_strncpy_1 (char *p, size_t n) +{ + call_strncpy_1 (p, p, n); +} diff --git a/gcc/testsuite/c-c++-common/analyzer/strncpy-1.c b/gcc/testsuite/c-c++-common/analyzer/strncpy-1.c new file mode 100644 index 000000000000..3ca1d81d90cb --- /dev/null +++ b/gcc/testsuite/c-c++-common/analyzer/strncpy-1.c @@ -0,0 +1,157 @@ +/* See e.g. https://en.cppreference.com/w/c/string/byte/strncpy */ + +/* { dg-additional-options "-Wno-stringop-overflow" } */ +/* { dg-additional-options "-fpermissive" { target c++ } } */ + +#include "../../gcc.dg/analyzer/analyzer-decls.h" + +typedef __SIZE_TYPE__ size_t; + +extern char *strncpy (char *dst, const char *src, size_t count); + +char * +test_passthrough (char *dst, const char *src, size_t count) +{ + char *result = strncpy (dst, src, count); + __analyzer_eval (result == dst); /* { dg-warning "TRUE" } */ + return result; +} + +char * +test_null_dst (const char *src, size_t count) +{ + return strncpy (NULL, src, count); /* { dg-warning "use of NULL where non-null expected" } */ +} + +char * +test_null_src (char *dst, size_t count) +{ + return strncpy (dst, NULL, count); /* { dg-warning "use of NULL where non-null expected" } */ +} + +void +test_zero_fill (char *dst) +{ + strncpy (dst, "", 5); + __analyzer_eval (dst[0] == '\0'); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + __analyzer_eval (dst[1] == '\0'); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + __analyzer_eval (dst[2] == '\0'); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + __analyzer_eval (dst[3] == '\0'); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + __analyzer_eval (dst[4] == '\0'); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + __analyzer_eval (__analyzer_get_strlen (dst) == 0); /* { dg-warning "TRUE" } */ + __analyzer_eval (__analyzer_get_strlen (dst + 1) == 0); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ +} + +char *test_unterminated_concrete_a (char *dst) +{ + char buf[3] = "abc"; /* { dg-warning "initializer-string for '\[^\n\]*' is too long" "" { target c++ } } */ + /* Should be OK to copy nothing. */ + return strncpy (dst, buf, 0); /* { dg-bogus "" } */ +} + +char *test_unterminated_concrete_b (char *dst) +{ + char buf[3] = "abc"; /* { dg-warning "initializer-string for '\[^\n\]*' is too long" "" { target c++ } } */ + /* Should be OK as the count limits the accesses to valid + locations within src buf. */ + return strncpy (dst, buf, 3); /* { dg-bogus "" } */ +} + +char *test_unterminated_concrete_c (char *dst) +{ + char buf[3] = "abc"; /* { dg-warning "initializer-string for '\[^\n\]*' is too long" "" { target c++ } } */ + /* Should warn: the count is one too high to limit the accesses + to within src buf. */ + return strncpy (dst, buf, 4); /* { dg-warning "stack-based buffer over-read" } */ +} + +char *test_terminated_concrete_d (char *dst) +{ + char buf[6]; + __builtin_memset (buf, 'a', 3); + __builtin_memset (buf + 3, 'b', 3); + + /* Shouldn't warn. */ + return strncpy (dst, buf, 6); /* { dg-bogus "" } */ +} + +char *test_unterminated_concrete_e (char *dst) +{ + char buf[6]; + __builtin_memset (buf, 'a', 3); + __builtin_memset (buf + 3, 'b', 3); + + /* Should warn. */ + return strncpy (dst, buf, 7); /* { dg-warning "stack-based buffer over-read" } */ +} + +char *test_unterminated_symbolic (char *dst, size_t count) +{ + char buf[3] = "abc"; /* { dg-warning "initializer-string for '\[^\n\]*' is too long" "" { target c++ } } */ + return strncpy (dst, buf, count); +} + +char *test_terminated_symbolic (char *dst, size_t count) +{ + const char *src = "abc"; + return strncpy (dst, src, count); /* { dg-bogus "" } */ +} + +char *test_uninitialized_concrete_a (char *dst) +{ + char buf[16]; + return strncpy (dst, buf, 0); /* { dg-bogus "" } */ +} + +char *test_uninitialized_concrete_b (char *dst) +{ + char buf[16]; + return strncpy (dst, buf, 1); /* { dg-warning "use of uninitialized value" } */ +} + +char *test_initialized_concrete_c (char *dst) +{ + char buf[16]; + buf[0] = 'a'; + return strncpy (dst, buf, 1); /* { dg-bogus "" } */ +} + +char *test_uninitialized_symbolic (char *dst, size_t count) +{ + char buf[16]; + return strncpy (dst, buf, count); /* { dg-warning "use of uninitialized value" } */ +} + +void test_truncation_1 (const char *src) +{ + char buf[16]; + strncpy (buf, src, 16); + /* buf might not be terminated (when strlen(src) > 16). */ + __analyzer_get_strlen (buf); /* { dg-warning "stack-based buffer over-read" "" { xfail *-*-* } } */ +} + +void test_truncation_2 (size_t count) +{ + char buf[16]; + strncpy (buf, "abc", count); + /* buf might not be terminated (when count <= 3). */ + __analyzer_get_strlen (buf); /* { dg-warning "stack-based buffer over-read" "" { xfail *-*-* } } */ +} + +void test_too_big_concrete (void) +{ + char buf[10]; + strncpy (buf, "abc", 128); /* { dg-warning "stack-based buffer overflow" } */ +} + +void test_too_big_symbolic (const char *src) +{ + char buf[10]; + strncpy (buf, src, 128); /* { dg-warning "stack-based buffer overflow" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/null-terminated-strings-1.c b/gcc/testsuite/gcc.dg/analyzer/null-terminated-strings-1.c index ecd794a2337a..c9095fa3b94a 100644 --- a/gcc/testsuite/gcc.dg/analyzer/null-terminated-strings-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/null-terminated-strings-1.c @@ -144,3 +144,27 @@ void test_casts (void) __analyzer_eval (__analyzer_get_strlen (p) == 0); /* { dg-warning "UNKNOWN" } */ __analyzer_eval (__analyzer_get_strlen (p + 1) == 0); /* { dg-warning "UNKNOWN" } */ } + +void test_filled_nonzero (void) +{ + char buf[10]; + __builtin_memset (buf, 'a', 10); + __analyzer_get_strlen (buf); /* { dg-warning "stack-based buffer over-read" "" { xfail *-*-* } } */ +} + +void test_filled_zero (void) +{ + char buf[10]; + __builtin_memset (buf, 0, 10); + __analyzer_eval (__analyzer_get_strlen (buf) == 0); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ + __analyzer_eval (__analyzer_get_strlen (buf + 1) == 0); /* { dg-warning "TRUE" "correct" { xfail *-*-* } } */ + /* { dg-bogus "UNKNOWN" "status quo" { xfail *-*-* } .-1 } */ +} + +void test_filled_symbolic (int c) +{ + char buf[10]; + __builtin_memset (buf, c, 10); + __analyzer_eval (__analyzer_get_strlen (buf) == 0); /* { dg-warning "UNKNOWN" } */ +} -- 2.26.3