Hi David, thanks for your review!
> One legal prerequisite is that you need to either
> (a) add the Signed-off-by tag to certify that you can contribute it, as
> per https://developercertificate.org/ (relatively easy), or
> (b) jump through the legal hoops described here:
> https://gcc.gnu.org/contribute.html#legal (rather harder).
For the time being, I will just use the Signed-off-by tag, as the
assignment process probably takes a long time. I would like to obtain
the documentation to assign copyright to the FSF for all my future
contributions to GCC, as I have the intention to contribute for the long
term.
I have updated the patch following your suggestions, I'll send it as a
separate email. Here I will reply to some of the review comments.
> Out of interest, how long did the bootstrap and testing take you? If
> that's a bottleneck, we may be able to set you up on a faster machine
> on the compile farm.
Around 7 hours or so, I'm not sure of the exact number of hours, both
times that I have done it, I left it overnight and didn't pay much
attention to the total time. Having access to a more powerful build
machine would be quite helpful.
> We regenerate this file from the .opt files and the generated HTML via
> "make regenerate-opt-urls"; given that there aren't any docs for the
> new options and that there's a typo in the 2nd one, it looks like you
> hand-edited this instead.
Yes, I wasn't aware that it was automatically generated, and found it
while grepping for analyzer warnings. I fixed that in my updated patch,
together with the Texinfo docs.
>> + return ctxt.warn ("%qE template does not end with XXXXXX",
m_fndecl);
>
> Maybe say "template string" rather than just "template", to avoid
> confusion in C++ sources? (not sure about this one)
I agree, "template string" is less ambiguous.
>> + void
>> + impl_call_pre (const call_details &cd) const final override
>> + {
>> + const svalue *ptr_sval = cd.get_arg_svalue (0);
>> + region_model_context *ctxt = cd.get_ctxt ();
>> + region_model *model = cd.get_model ();
>> +
>> + const svalue *strlen_sval
>> + = model->check_for_null_terminated_string_arg (cd, 0, false,
>> nullptr);
>> + if (!strlen_sval)
>> + {
>> + if (ctxt)
>> + ctxt->terminate_path ();
>> + return;
>> + }
>> +
>> + if (cd.get_arg_string_literal (0))
>> + {
>> + if (ctxt)
>> + ctxt->warn (std::make_unique<mkstemp_of_string_literal> (cd));
>> + }
>> + else if (check_suffix (cd, ptr_sval, strlen_sval).is_false ())
>> + {
>> + if (ctxt)
>> + ctxt->warn (std::make_unique<mkstemp_missing_suffix> (cd));
>> + }
>
> This looks correct, but it would be cleaner to do the
> if (ctxt)
> once, and have it guard a block that all the checks for problems live
> within. ctxt is non-null when the exploded graph is being built, but
> is null during feasibility checking. You can only store a warning if
> you have a non-null ctxt, so it's wasted work to check for a problem if
> there's no ctxt to report it to.
I wasn't too sure under what conditions `ctxt` may be null, so thanks
for the explanation. I have added an early return if `ctxt` is null,
and removed the other checks.
>> + /* Return true if the template ends with "XXXXXX", false if it
>> + definitely does not, or unknown if we can't determine. */
>> + static tristate
>> + check_suffix (const call_details &cd, const svalue *ptr_sval,
>> + const svalue *strlen_sval)
>> + {
>> + region_model *model = cd.get_model ();
>> +
>> + const constant_svalue *len_cst =
>> strlen_sval->dyn_cast_constant_svalue ();
>> + if (!len_cst)
>> + return tristate::TS_UNKNOWN;
>> +
>> + byte_offset_t len = TREE_INT_CST_LOW (len_cst->get_constant ());
>> + if (len < 6)
>> + return tristate::TS_FALSE;
>> +
>> + tree arg_tree = cd.get_arg_tree (0);
>> + const region *reg
>> + = model->deref_rvalue (ptr_sval, arg_tree, cd.get_ctxt ());
>> + const region *base_reg = reg->get_base_region ();
>> + const svalue *base_sval
>> + = model->get_store_value (base_reg, cd.get_ctxt ());
>> +
>> + const constant_svalue *cst_sval =
>> base_sval->dyn_cast_constant_svalue ();
>> + if (!cst_sval)
>> + return tristate::TS_UNKNOWN;
>> +
>> + tree cst = cst_sval->get_constant ();
>> + if (TREE_CODE (cst) != STRING_CST)
>> + return tristate::TS_UNKNOWN;
>> +
>> + HOST_WIDE_INT str_len = len.to_shwi ();
>> + if (1 + str_len > TREE_STRING_LENGTH (cst))
>> + return tristate::TS_UNKNOWN;
>> +
>> + if (memcmp (TREE_STRING_POINTER (cst) + str_len - 6, "XXXXXX",
6) != 0)
>> + return tristate::TS_FALSE;
>
> I'm not 100% convinced the logic here is correct, but I can't see a way
> to make it overflow the TREE_STRING_POINTER (cst) buffer.
I can't see a way to make it overflow either, but you were right in
doubting the logic. I've realized that it would generate a spurious
warning in cases like the following:
char buf[] = "/tmp/XXXXXX";
mkstemp(buf + 5);
`strlen_sval` would be 6, but base_sval (and ultimately the buffer)
would refer to "/tmp/XXXXXX". So
memcmp(TREE_STRING_POINTER(cst) + 0, "XXXXXX", 6)
Would compare "/tmp/X" against "XXXXXX", and an incorrect warning would
be emitted.
I have written a new test case to see if this was actually the case (it
was), and fixed it.
>> +void test_NULL (void)
>> +{
>> + mkstemp (NULL); /* possibly -Wanalyzer-null-argument */
>
> Indeed, check_for_null_terminated_string_arg would ideally check for
> this (I thought it did, but I see the TODO comment in the source; oh
> well).
I didn't think of it myself, but saw it on the putenv tests :-)
> This patch is close to being ready, but, to repeat something I
> mentioned in an email to another candidate, right now we're in feature
> freeze for GCC 16, trying to focus on stabilizing for the release. We
> call this "stage 4" of the release cycle. Hence I plan to wait before
> actually pushing the patch. Hopefully we'll transition to stage 1 of
> the GCC 17 release cycle sometime next month (when we're open to new
> features).
That's not a problem, but I have a question related to it. Once the
patch is considered done, it becomes very easy to add support for
`mkostemp`, `mkstemps`, `mkostemps`, and `mkdtemp`. If the patch is not
yet merged, what would be the correct way to go about it? I could
update the current patch to support those, but I would prefer to
consider it "done" (once it is) and add support for the related
functions in a different patch. Is it better to just wait, or is there
some way to both consider this patch done and continue working based on it?
> Thanks again for the patch; hope this is constructive
It is, thank you for the review.
Tomás
