On Wed, 2026-03-11 at 14:02 +0100, Tomás Ortín Fernández (quanrong) wrote:
> This patch adds two new analyzer warnings for misuse of mkstemp(3):
>
Thanks for the updated patch.
[...snip...]
> +private:
> + const gimple &m_call_stmt; // non-NULL
Getting nitpicky, but a reference captures "non-nullness" in the C++
type system so the above comment is redundant.
> + tree m_fndecl; // non-NULL
whereas "tree" is a typedef to a pointer so this makes sense.
> +class kf_mkstemp : public known_function
> +{
> +public:
> + bool
> + matches_call_types_p (const call_details &cd) const final override
> + {
> + return (cd.num_args () == 1 && cd.arg_is_pointer_p (0));
> + }
> +
> + void
> + impl_call_pre (const call_details &cd) const final override
> + {
> + region_model_context *ctxt = cd.get_ctxt ();
> + /* If there's no context we can't store warnings, so we return
> early. */
> + if (!ctxt)
> + {
> + cd.set_any_lhs_with_defaults ();
> + return;
> + }
Hmmm, I don't like the way the patch has
cd.set_any_lhs_with_defaults ();
above and then repeats it below at the end of the function (DRY
principle). How about introducing a subroutine within the class to do
the checks, so it looks like:
if (ctxt)
if (!check_argument (cd))
return
cd.set_any_lhs_with_defaults ();
or somesuch? Or does that make things messier?
> +
> + const svalue *ptr_sval = cd.get_arg_svalue (0);
> + region_model *model = cd.get_model ();
> +
> + const svalue *strlen_sval
> + = model->check_for_null_terminated_string_arg (cd, 0, false,
> nullptr);
> + if (!strlen_sval)
> + {
> + ctxt->terminate_path ();
> + return;
> + }
> +
> + if (cd.get_arg_string_literal (0))
> + {
> + ctxt->warn (std::make_unique<mkstemp_of_string_literal> (cd));
> + }
> + else if (check_suffix (cd, ptr_sval, strlen_sval).is_false ())
> + {
> + ctxt->warn (std::make_unique<mkstemp_missing_suffix> (cd));
> + }
> +
> + cd.set_any_lhs_with_defaults ();
> + }
> +
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c
> b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c
> new file mode 100644
> index 00000000000..d8e0e51c39f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c
> @@ -0,0 +1,96 @@
> +/* { dg-additional-options "-Wno-analyzer-null-argument" } */
> +/* { dg-skip-if "has no putenv" { "avr-*-*" } } */
The above skip-if line looks like it was a copy&paste from a test of
"putenv"; it means that the test should be skipped on targets matching
avr-*-* since they don't have "putenv".
There might be targets that don't have mkstemp, but until we discover
them, best to delete that line for this test.
Other than those nitpicks, the patch looks great; thanks
Dave
