On Wed, 2026-03-25 at 19:20 +0000, Saish Sandip Kambali wrote:
> This patch implements known_function handling for strnlen and
> __builtin_strnlen within the analyzer, it uses bifurcation logic
> for truncated and full read conditions.
Hi Saish, thanks for resending the patch.
What testing did you do on the patch? (please state this when posting
a patch)
Overall the patch promising, but it needs a little improvement; various
comments inline below...
[...snip...]
> diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
> index b1ccbd6584a..a292e220c1e 100644
> --- a/gcc/analyzer/kf.cc
> +++ b/gcc/analyzer/kf.cc
> @@ -1710,6 +1710,194 @@ public:
> }
> };
>
> +
> +/* Handles strnlen by splitting into two outcomes:
> + a) Truncated read: The limit 'n' is reached before the null-terminator,
> + in this case the result is 'n'.
> + b) Full read: The null-terminator is found before or at the limit n,
> + in this case the result is the number of bytes read before the
> + null-terminator. */
AIUI, the input string to strnlen doesn't need to be null-terminated;
it just needs to be valid to read up to n bytes from it.
So in (a) above it should probably say "any null-terminator" rather
than "the null-terminator", and (b) should probably say "A null
terminator is found before...etc".
> +
> +class kf_strnlen : public builtin_known_function
> +{
> +public:
> + bool matches_call_types_p (const call_details &cd) const final override
> + {
> + return (cd.num_args () == 2
> + && cd.arg_is_pointer_p (0)
> + && cd.arg_is_integral_p (1));
> + }
> +
> + enum built_in_function builtin_code () const final override
> + {
> + return BUILT_IN_STRNLEN;
> + }
> +
> + void impl_call_pre (const call_details &cd) const final override;
> + void impl_call_post (const call_details &cd) const final override;
> +};
> +
> +void
> +kf_strnlen::impl_call_pre (const call_details &cd) const
> +{
> + cd.check_for_null_terminated_string_arg (0);
check_for_null_terminated_string_arg will complain if the argument
isn't null terminated, and I don't think we want that.
I think this can be handled instead in the impl_call_post cases you
have below; the untruncated read case should do something to complain
if we have an untruncated string. From the test case coverage you
have, it looks like the patch is already doing that (presumably due to
the read_bytes check at the end).
> +}
> +
> +void
> +kf_strnlen::impl_call_post (const call_details &cd) const
> +{
> + class strnlen_call_info : public call_info
> + {
> + public:
> + strnlen_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)
> + {
> + }
> +
> + void print_desc (pretty_printer &pp) const final override
> + {
> + if (m_truncated_read)
> + pp_printf (&pp, "when %qE reaches the maximum limit", get_fndecl ());
> + else
> + pp_printf (&pp, "when %qE finds the null terminator", 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));
> +
> + /* Arguments at index '0' is the string pointer. */
> + const svalue *src_sval = cd.get_arg_svalue (0);
> + const region *src_reg
> + = model->deref_rvalue (src_sval, cd.get_arg_tree (0), ctxt);
> +
> + /* Argument 1 is the limit n. */
> + const svalue *limit_n_sval = cd.get_arg_svalue (1);
> +
> + /* Symbolic values for return value of the call. */
> + const svalue *result_sval = nullptr;
> + const svalue *bytes_to_check = nullptr;
> +
> + /* Truncated read. */
> + if (m_truncated_read)
> + {
> + result_sval = limit_n_sval;
> +
> + if (m_num_bytes_with_terminator_sval)
> + {
> + /* The null-terminator is not in the bounds of n. */
> + if (!model->add_constraint (m_num_bytes_with_terminator_sval,
> + GT_EXPR, limit_n_sval, ctxt))
> + return false;
> + }
> + else
> + {
> + }
Should lose this redundant "else" block.
> + }
> + else
> + {
> + if (m_num_bytes_with_terminator_sval)
> + {
> + /* The string length is less than or equal to n. */
> + if (!model->add_constraint (m_num_bytes_with_terminator_sval,
> + LE_EXPR, limit_n_sval, ctxt))
> + return false;
> + }
> + else
> + {
> + /* This bifurcation part is removed once failed. */
> + return false;
> + }
> + }
> +
> + /* Computing return value only if lhs is present. */
> + if (tree lhs_type = cd.get_lhs_type ())
> + {
> + if (!m_truncated_read && m_num_bytes_with_terminator_sval)
> + {
> + /* Since the strnlen() returns the count of bytes excluding \0.
> */
> + result_sval = model->get_manager ()->get_or_create_binop (
> + lhs_type, MINUS_EXPR, m_num_bytes_with_terminator_sval,
> + model->get_manager ()->get_or_create_constant_svalue (
> + lhs_type, build_int_cst (lhs_type, 1)));
> + }
> +
> + /* This sets the return value, if the lhs type is decalred. */
Typo: "declared", presumably.
> + if (result_sval)
> + {
> + cd.maybe_set_lhs (result_sval);
> + }
> + else
> + {
> + }
We should lose this redundant "else" clause. Also, our coding standard
is to omit the '{' and '}' where they are redundant, which is the case
here for the "if" clause, so all this should simply read:
if (result_sval)
cd.maybe_set_lhs (result_sval);
> + }
> +
> + /* This checks for buffer over read, and poison bytes as a safety check
> + for both the lhs data type for null and non-null. */
> + bytes_to_check = m_truncated_read ? limit_n_sval
> + : m_num_bytes_with_terminator_sval;
> +
> + if (bytes_to_check)
> + {
> + model->read_bytes (src_reg, cd.get_arg_tree (0), bytes_to_check,
> ctxt);
> + }
Similar comment about redundant { and }
> +
> + return true;
> + }
> +
> + private:
> + /* This symbolic value representing the number of bytes read upto the
> + first non-terminating character, it will be a null pointer. */
"Up to and including", I believe.
Looks like this comment is missing something - either say *when* it
will be a null pointer, or (better) simply say it *can* be a null
pointer.
> + const svalue *m_num_bytes_with_terminator_sval;
> +
> + /* If true: We are simulating 'truncated read'
> + If false: We are simulating the 'full read'. */
> + bool m_truncated_read;
> + };
> +
> + const svalue *limit_n_sval = cd.get_arg_svalue (1);
> +
> + if (tree n_cst = limit_n_sval->maybe_get_constant ())
> + {
> + if (zerop (n_cst))
> + {
> + cd.maybe_set_lhs (limit_n_sval);
> + return;
> + }
> + }
> +
> + /* Body of kf_strnlen::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 reported. */
> + const region_model *model = cd.get_model ();
> + const svalue *ptr_arg_sval = cd.get_arg_svalue (0);
> + const region *buf_reg
> + = model->deref_rvalue (ptr_arg_sval, cd.get_arg_tree (0), nullptr);
> +
> + const svalue *num_bytes_with_terminator_sval
> + = model->scan_for_null_terminator (buf_reg, cd.get_arg_tree (0),
> + nullptr, nullptr);
> + cd.get_ctxt ()->bifurcate (
> + std::make_unique<strnlen_call_info> (cd,
> + num_bytes_with_terminator_sval,
> + false));
> + cd.get_ctxt ()->bifurcate (
> + std::make_unique<strnlen_call_info> (cd,
> + num_bytes_with_terminator_sval,
> + true));
> + cd.get_ctxt ()->terminate_path ();
> + }
> +}
> +
[...snip...]
>
> diff --git a/gcc/testsuite/gcc.dg/analyzer/strnlen-1.c
> b/gcc/testsuite/gcc.dg/analyzer/strnlen-1.c
> new file mode 100644
> index 00000000000..78cfe0c0629
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/strnlen-1.c
> @@ -0,0 +1,121 @@
> +#include "analyzer-decls.h"
> +
> +typedef __SIZE_TYPE__ size_t;
Please add a "test_passthrough" (grep for other examples).
> +
> +/* noinline wrapper of test_string. */
> +static size_t __attribute__((noinline))
> +call_strnlen_1 (const char *p, size_t n)
> +{
> + return __builtin_strnlen (p, n);
> +}
> +
> +void test_string (void)
> +{
> + /* case 1: n is greater than string length */
> + __analyzer_eval (call_strnlen_1 ("abc", 10) == 3); /* { dg-warning "TRUE"
> } */
> +
> + /* case 2: n is smaller than the string length */
> + __analyzer_eval (call_strnlen_1 ("abc", 2) == 2); /* { dg-warning "TRUE" }
> */
> +
> + /* case 3: n is exactly the string length */
> + __analyzer_eval (call_strnlen_1 ("abc", 3) == 3); /* { dg-warning "TRUE" }
> */
> +
> + /* case 4: n is zero and should return 0 immediately. */
> + __analyzer_eval (call_strnlen_1 ("abc", 0) == 0); /* { dg-warning "TRUE" }
> */
> +}
> +
> +/* checks if pointer is NULL */
> +void test_null_1 (void)
> +{
> + __builtin_strnlen (NULL, 1); /* { dg-warning "use of NULL where non-null
> expected" } */
> +}
> +
> +void test_null_0 (void)
> +{
> + /* n == 0 should not trigger a NULL warning in strnlen. */
> + __analyzer_eval (__builtin_strnlen (NULL, 0) == 0); /* { dg-warning "TRUE"
> } */
> +}
> +
> +/* buffer declared but not initialized yet, warning if read uninitialized
> values */
> +void test_uninitialized (size_t n)
> +{
> + char buf[16];
> + if (n > 0)
> + __builtin_strnlen (buf, n); /* { dg-warning "use of uninitialized value"
> } */
> +}
> +
> +/* buffer is declared and partially initialized, warning if read
> uninitialized values */
> +void test_partially_initialized (void)
> +{
> + char buf[16];
> + buf[0] = 'a';
> + __builtin_strnlen (buf, 2); /* { dg-warning "use of uninitialized value" }
> */
Would be good to also have
__builtin_strnlen (buf, 1);
first, to verify that we *don't* warn for this case.
> +}
> +
> +/* noinline wrapper of test_unterminated case 1 */
> +static size_t __attribute__((noinline))
> +call_strnlen_2 (const char *p, size_t n)
> +{
> + return __builtin_strnlen (p, n); /* { dg-warning "stack-based buffer
> over-read" } */
>
Does the dg-warning here actually fire? My reading of
test_unterminated below is that call_strnlen_2 is used in a correct way
and thus we shouldn't be emitting a warning here.
> +}
> +
> +/* noinline wrapper of test_unterminated case 2 */
> +static size_t __attribute__((noinline))
> +call_strnlen_3 (const char *p, size_t n)
> +{
> + return __builtin_strnlen (p, n); /* { dg-warning "stack-based buffer
> over-read" } */
> +}
> +
> +void test_unterminated (void)
> +{
> + const char buf[3] = {'a', 'b', 'c'};
> +
> + /* case 1: SAFE as strnlen stops at n = 3, so never looked for \0 */
> + size_t result = call_strnlen_2 (buf, 3);
> + __analyzer_eval (result == 3); /* { dg-warning "TRUE" } */
> +
> + /* case 2: Unsafe as n=4 forces the analyzer to look past the 3 bytes
> buffer */
> + call_strnlen_3 (buf, 4);
> +}
> +
> +/* noinline wrapper of test_max_n */
> +static size_t __attribute__((noinline))
> +call_strnlen_4 (const char *p, size_t n)
> +{
> + return __builtin_strnlen (p, n);
> +}
> +
> +/* for abstraction, assures the length l < n */
> +void test_abstract (const char *s, size_t n)
> +{
> + if (s == NULL)
> + return;
> + size_t length = call_strnlen_4 (s, n);
> +
> + __analyzer_eval (length <= n); /* { dg-warning "UNKNOWN" } */
> +}
> +
> +/* noinline wrapper of test_array_initialization, case: true */
> +static size_t __attribute__((noinline))
> +call_strnlen_5 (const char *p, size_t n)
> +{
> + return __builtin_strnlen (p, n);
> +}
> +
> +/* noinline wrapper of test_array_initialized, case: unknown */
> +static size_t __attribute__((noinline))
> +call_strnlen_5a (const char *p, size_t n)
> +{
> + return __builtin_strnlen (p, n); /* { dg-warning "stack-based buffer
> over-read" } */
> +}
> +
> +/* pointer arithmetic can be performed and warnings for over reading buffer
> */
> +void test_array_initialization_implicit_length (void)
> +{
> + const char buf[] = "abc";
> +
> + __analyzer_eval (call_strnlen_5 (buf, 10) == 3); /* { dg-warning "TRUE" }
> */
> + __analyzer_eval (call_strnlen_5 (buf + 2, 3) == 1); /* { dg-warning "TRUE"
> } */
> + __analyzer_eval (call_strnlen_5 (buf + 3, 3) == 0); /* { dg-warning "TRUE"
> } */
> + __analyzer_eval (call_strnlen_5a (buf + 4, 3) == 0); /* { dg-warning
> "FALSE" } */
> +}
Thanks; overall, the test coverage looks good; I think you covered
everything I was hoping for (other than the points noted above).
Hope the above is constructive and makes sense; thanks again for the
patch
Dave
