On Mon, 2026-03-16 at 16:02 +0100, Tomás Ortín Fernández wrote:
Hi Tomás, thanks for the patch. Overall looks good, but various
comments inline below...
[...snip...]
> diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
> index 6ecdf5c4006..73d5966dfcd 100644
> --- a/gcc/analyzer/kf.cc
> +++ b/gcc/analyzer/kf.cc
> @@ -26,6 +26,7 @@ along with GCC; see the file COPYING3. If not see
> #include "analyzer/region-model.h"
> #include "analyzer/call-details.h"
> #include "analyzer/call-info.h"
> +#include "tree.h"
"tree.h" is included by analyzer/common.h so the above line should be
unnecessary.
[...snip...]
> +void
> +kf_mktemp_family::check_template_with_suffixlen_arg (
> + const call_details &cd, unsigned int suffixlen_arg_idx)
> +{
> + const svalue *suffixlen_sval = cd.get_arg_svalue (suffixlen_arg_idx);
> + const constant_svalue *cst_sval
> + = suffixlen_sval->dyn_cast_constant_svalue ();
> + if (!cst_sval)
> + {
> + /* Suffix length unknown, but we still need to check whether the
> template
> + argument is a null-terminated string. */
> + region_model *model = cd.get_model ();
> + model->check_for_null_terminated_string_arg (cd, 0, false, nullptr);
> + return;
> + }
> +
> + tree cst = cst_sval->get_constant ();
> + /* TODO: Negative suffixlen is always wrong and potentially OOB,
> maybe add a
> + warning in the future? */
> + if (tree_int_cst_sgn (cst) < 0)
> + return;
I wonder if this would reliably generate EINVAL on all implementations?
Let's leave any warning about this to a followup.
> +
> + unsigned HOST_WIDE_INT suffixlen = TREE_INT_CST_LOW (cst);
> + check_template (cd, suffixlen);
> +}
> +
> +void
> +kf_mktemp_family::check_template (const call_details &cd, size_t
> trailing_len)
> +{
> + region_model_context *ctxt = cd.get_ctxt ();
> + gcc_assert (ctxt);
> + 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)
> + return;
> +
> + if (cd.get_arg_string_literal (0))
> + {
> + ctxt->warn (std::make_unique<mktemp_of_string_literal> (cd));
> + }
> + else if (check_placeholder (cd, trailing_len, ptr_sval, strlen_sval)
> + .is_false ())
> + {
> + ctxt->warn (
> + std::make_unique<mktemp_missing_placeholder> (cd, trailing_len));
> + }
Note that (for better or worse) our coding standard is to omit
redundant { } blocks, so the above can be written:
if (cd.get_arg_string_literal (0))
ctxt->warn (std::make_unique<mktemp_of_string_literal> (cd));
else if (check_placeholder (cd, trailing_len, ptr_sval, strlen_sval)
.is_false ())
ctxt->warn (std::make_unique<mktemp_missing_placeholder>
(cd, trailing_len));
which might save a little horizontal space.
> +}
> +
> +void
> +kf_mktemp_family::check_flags (const call_details &cd,
> + unsigned int flags_arg_idx)
> +{
> + region_model_context *ctxt = cd.get_ctxt ();
> + gcc_assert (ctxt);
> +
> + const svalue *flags_sval = cd.get_arg_svalue (flags_arg_idx);
> + const constant_svalue *cst = flags_sval->dyn_cast_constant_svalue ();
> + if (!cst)
> + return;
>
> - int mkstemp(char *template);
> + unsigned HOST_WIDE_INT flags = TREE_INT_CST_LOW (cst->get_constant ());
> +
> + /* Check whether any of the implicit flags are redundantly specified. */
> + unsigned HOST_WIDE_INT implicit_flags = 0;
> + for (const char *name : { "O_RDWR", "O_CREAT", "O_EXCL" })
> + if (tree cst_tree = get_stashed_constant_by_name (name))
> + implicit_flags |= TREE_INT_CST_LOW (cst_tree);
Note that get_stashed_constant_by_name can fail and return null
(see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108708), but the way
you've written this, it's OK; the pertinent bits of implicit_flags
won't get set and so the warning won't fire.
> +
> + if (flags & implicit_flags)
> + ctxt->warn (std::make_unique<mkostemp_redundant_flags> (cd));
> +}
> +
[...snip...]
> +
> +class kf_mkostemp : public kf_mktemp_family
> +{
> +public:
> + bool
> + matches_call_types_p (const call_details &cd) const final override
> + {
> + return (cd.num_args () == 2 && cd.arg_is_pointer_p (0));
check_flags probably assumes that it's dealing with an integer type,
so matches_call_types_p here should probably also check that:
cd.arg_is_integral_p (1)
Otherwise we might crash if given a (nonstandard) decl:
int mkostemp (void *, void *);
where the 2nd argument isn't an integer type.
This doesn't tend to happen on real code, but fuzz testing tends to
generate crashes of kf-handlers that don't check all of the type
assumptions they're making.
> + }
> +
> + void
> + impl_call_pre (const call_details &cd) const final override
> + {
> + if (cd.get_ctxt ())
> {
> - ctxt->warn (std::make_unique<mkstemp_missing_suffix> (cd));
> + check_template (cd, 0);
> + check_flags (cd, 1);
> }
> +
> + cd.set_any_lhs_with_defaults ();
This is something I probably should have pointed out for the first
patch (sorry!), but it would be good to bifurcate the analysis for
these functions into "call succeeded" and "call failed" cases, with
region_model::set_errno on the failures. This tends to be useful for
detecting missing error-checking in the program being analyzed. This
is done in the impl_call_post handler (rather than _pre).
Let's save that for a further followup since this patch is already
rather large.
[...snip...]
> +class kf_mkostemps : public kf_mktemp_family
> +{
> +public:
> + bool
> + matches_call_types_p (const call_details &cd) const final override
> + {
> + return (cd.num_args () == 3 && cd.arg_is_pointer_p (0));
As above, let's check the types of all 3 args here.
[...snip...]
> +class kf_mkstemps : public kf_mktemp_family
> +{
> +public:
> + bool
> + matches_call_types_p (const call_details &cd) const final override
> + {
> + return (cd.num_args () == 2 && cd.arg_is_pointer_p (0));
Likewise here.
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/mkostemp-1.c
[...snip...]
> +void test_redundant_o_rdwr (void)
> +{
> + char tmpl[] = "/tmp/testXXXXXX";
> + mkostemp (tmpl, O_RDWR); /* { dg-warning "'mkostemp' flags argument
> should not include 'O_RDWR', 'O_CREAT', or 'O_EXCL' as these are already
> implied" } */
> +}
Note that getting a stashed constant can currently fail depending on
the system headers; see e.g.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108708
So we might want to add some kind of dg-skip on some systems (difficult
to say ahead of time though).
> +
> +void test_redundant_o_creat (void)
> +{
> + char tmpl[] = "/tmp/testXXXXXX";
> + mkostemp (tmpl, O_CREAT); /* { dg-warning "'mkostemp' flags argument
> should not include 'O_RDWR', 'O_CREAT', or 'O_EXCL' as these are already
> implied" } */
> +}
Likewise.
> +
> +void test_redundant_o_excl (void)
> +{
> + char tmpl[] = "/tmp/testXXXXXX";
> + mkostemp (tmpl, O_EXCL); /* { dg-warning "'mkostemp' flags argument
> should not include 'O_RDWR', 'O_CREAT', or 'O_EXCL' as these are already
> implied" } */
> +}
Likewise.
> +
> +void test_redundant_combined (void)
> +{
> + char tmpl[] = "/tmp/testXXXXXX";
> + mkostemp (tmpl, O_RDWR | O_CREAT); /* { dg-warning "'mkostemp' flags
> argument should not include 'O_RDWR', 'O_CREAT', or 'O_EXCL' as these
> are already implied" } */
> +}
Likewise.
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/mkostemps-1.c
> b/gcc/testsuite/gcc.dg/analyzer/mkostemps-1.c
> new file mode 100644
> index 00000000000..4e259c67a8f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/mkostemps-1.c
> @@ -0,0 +1,126 @@
> +/* { dg-additional-options "-Wno-analyzer-null-argument" } */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <fcntl.h>
> +
> +extern int mkostemps (char *, int, int);
> +extern void populate (char *buf);
> +
> +void test_passthrough (char *s)
> +{
> + mkostemps (s, 3, O_CLOEXEC);
> +}
Here, only the first param "s" seen by the known function is fully
symbolic, whereas it will see constant_svalues for args 2 and 3. For
the "passthrough" tests, it's best to do make all the params be
symbolic (and thus for test_passthrough to take 3 args), to maximize
the chance that we detect bugs where we forgot to handle non-constant
svalues. Doing all combinations of symbolic vs concrete is probably
overkill.
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/mkstemps-1.c
> b/gcc/testsuite/gcc.dg/analyzer/mkstemps-1.c
> new file mode 100644
> index 00000000000..c9172122c30
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/mkstemps-1.c
> @@ -0,0 +1,88 @@
> +/* { dg-additional-options "-Wno-analyzer-null-argument" } */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +extern void populate (char *buf);
> +
> +void test_passthrough (char *s)
> +{
> + mkstemps (s, 4);
> +}
> +
> +void test_string_literal_correct_placeholder (void)
> +{
> + mkstemps ("/tmp/fooXXXXXX.txt", 4); /* { dg-warning "'mkstemps' on a
> string literal \\\[STR30-C\\\]" } */
> + /* { dg-message "use a writable character array" "fix suggestion" {
> target *-*-* } .-1 } */
> +}
How about a test of:
mkstemps ("/tmp/fooXXXXXX.txt", 3);
where the user forgot to count the '.' between the "XXXXXX" and the
"txt"?
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/mktemp-1.c
> b/gcc/testsuite/gcc.dg/analyzer/mktemp-1.c
> new file mode 100644
> index 00000000000..19b565c5321
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/mktemp-1.c
> @@ -0,0 +1,99 @@
> +/* { dg-additional-options "-Wno-analyzer-null-argument" } */
> +
> +/* TODO: mktemp is deprecated per MSC24-C
> + (https://wiki.sei.cmu.edu/confluence/x/hNYxBQ).
> + Once a warning for deprecated functions exists, mktemp should
> + also warn about its use. */
I was thinking that we could just assume that the system header should
have:
__attribute__ ((deprecated)) on the decl of mktemp; see
https://gcc.gnu.org/onlinedocs/gcc/Common-Attributes.html#index-deprecated
but it looks like mktemp is sufficiently unsafe that we should probably
issue some kind of "use of inherently unsafe function" warning.
Let's leave that to a followup.
Thanks again for the patch; hope the above makes sense.
Dave
