Hi Dave, thanks for your review. I have corrected the issues you
pointed out and sent an updated patch. What follows are my comments on
some of the issues.
[...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.
Not on all of them. Some like Gnulib do generate EINVAL. But others
don't; for example, in `libiberty/mkstemps.c` , `mkstemps` doesn't check
whether suffix_len is negative:
if ((int) len < 6 + suffix_len
|| strncmp (&pattern[len - 6 - suffix_len], "XXXXXX", 6))
{
return -1;
}
There could be more cases like libiberty's, so that's why I think it
could be a good idea to add a warning. Maybe it could be made more
general so it can be used in similar cases, e.g. `ftruncate(int fd,
off_t length)` when passed a negative length.
+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.
I was aware that the standard calls for omitting { } when not strictly
needed, but I didn't know if the block could be added in some cases
where it may benefit readability (e.g. see the fragment from libiberty
above). Given that now I know it's a strict rule, I will remove the
brackets.
+}
+
+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.
My intention was for the `if` to guard from that case, as the result of
the expression containing the assignment would be evaluated to null.
Isn't that the case? It's true that the `if` could be omitted, but I
think skipping it when `get_stashed_constant_by_name` returns null is
more explicit.
[...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).
I added a couple of comments pointing that out for future reference. Is
there some kind of testing in multiple platforms already in place, or
are these issues uncovered when individual people test it on their
machine and see it fail?
[...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.
I've been thinking about this. MSC24-C lists both deprecated and
obsolescent functions. I think it could be reasonable to assume that
deprecated functions like `mktemp` have `__attribute__ ((deprecated))`,
as they have been removed from the standards. If someone is using a
library that doesn't mark it as deprecated, maybe it's reasonable to
assume it's likely an old library that doesn't have an alternative.
What do you think?
On the other hand, obsolescent functions are defined as such by MSC24-C,
and it's an opinionated list. I think it would make sense to add a
warning for MSC24-C, but it probably should be disabled by default.
What I'm not sure is how to handle MSC24-C warnings for deprecated
functions in that case: the attribute warning will already catch them if
present, but a user may want to know that MSC24-C also recommends not
using them, or may want to be warned about them when enabling MSC24-C
even in a codebase with old system headers.
Let's leave that to a followup.
Agreed.
Thanks again for the patch; hope the above makes sense.
Thanks for your review! It's very useful.
Tomás
