On Fri, 2023-09-01 at 00:04 +0200, priour...@gmail.com wrote:
> Hi,
>
> Succesfully regstrapped off trunk 7f2ed06ddc825e8a4e0edfd1d66b5156e6dc1d34
> on x86_64-linux-gnu.
>
> Is it OK for trunk ?
Hi Benjamin.
Thanks for the patch. It's OK as-is, but it doesn't cover every
case...
[...snip...]
> diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc
> index 66fb0fe871e..8d60e928b15 100644
> --- a/gcc/analyzer/call-details.cc
> +++ b/gcc/analyzer/call-details.cc
> @@ -295,6 +295,17 @@ call_details::get_arg_svalue (unsigned idx) const
> return m_model->get_rvalue (arg, m_ctxt);
> }
>
> +/* If argument IDX's svalue at the callsite is a region_svalue,
> + return the region it points to.
> + Otherwise return NULL. */
> +
> +const region *
> +call_details::maybe_get_arg_region (unsigned idx) const
> +{
> + const svalue *sval = get_arg_svalue (idx);
> + return sval->maybe_get_region ();
> +}
> +
Is this the correct thing to be doing? It's used in the following...
[...snip...]
> diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc
> index 393b4f25e79..4450892dfa2 100644
> --- a/gcc/analyzer/kf-lang-cp.cc
> +++ b/gcc/analyzer/kf-lang-cp.cc
[...snip...]
> @@ -54,28 +90,75 @@ public:
> region_model *model = cd.get_model ();
> region_model_manager *mgr = cd.get_manager ();
> const svalue *size_sval = cd.get_arg_svalue (0);
> - const region *new_reg
> - = model->get_or_create_region_for_heap_alloc (size_sval, cd.get_ctxt
> ());
> - if (cd.get_lhs_type ())
> + region_model_context *ctxt = cd.get_ctxt ();
> + const gcall *call = cd.get_call_stmt ();
> +
> + /* If the call was actually a placement new, check that accessing
> + the buffer lhs is placed into does not result in out-of-bounds. */
> + if (is_placement_new_p (call))
> {
> - const svalue *ptr_sval
> - = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> - cd.maybe_set_lhs (ptr_sval);
> + const region *ptr_reg = cd.maybe_get_arg_region (1);
> + if (ptr_reg && cd.get_lhs_type ())
> + {
...which will only fire if arg 1 is a region_svalue. This won't
trigger if you have e.g. a binop_svalue for pointer arithmetic.
What happens e.g. for this one-off-the-end bug:
void *p = malloc (4);
if (!p)
return;
int32_t *i = ::new (p + 1) int32_t;
*i = 42;
So maybe call_details::maybe_get_arg_region should instead be:
/* Return the region that argument IDX points to. */
const region *
call_details::deref_ptr_arg (unsigned idx) const
{
const svalue *ptr_sval = get_arg_svalue (idx);
return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
}
(caveat: I didn't test this)
> + const region *base_reg = ptr_reg->get_base_region ();
> + const svalue *num_bytes_sval = cd.get_arg_svalue (0);
> + const region *sized_new_reg
> + = mgr->get_sized_region (base_reg,
> + cd.get_lhs_type (),
> + num_bytes_sval);
Why do you use the base_reg here, rather than just ptr_reg?
In the example above, the *(p + 1) has base region
heap_allocated_region, but the ptr_reg is one byte higher; hence
check_region_for_write of 4 bytes ought to detect a problem with
writing 4 bytes to *(p + 1), but wouldn't complain about the write to
*p.
...assuming that I'm reading this code correctly.
> + model->check_region_for_write (sized_new_reg,
> + nullptr,
> + ctxt);
> + const svalue *ptr_sval
> + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
> + cd.maybe_set_lhs (ptr_sval);
> + }
> + }
[...snip...]
The patch is OK for trunk as is; but please can you look into the
above.
If the above is a problem, you can either do another version of the
patch, or do it as a followup patch (whichever you're more comfortable
with, but it might be best to get the patch into trunk as-is, given
that the GSoC period is nearly over).
Thanks
Dave
