Hi David,
On Fri, Sep 1, 2023 at 1:59 AM David Malcolm <dmalc...@redhat.com> wrote:
> On Fri, 2023-09-01 at 00:04 +0200, priour...@gmail.com wrote:
>
>
[..snip..]
> ...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.
>
>
Thanks for the test case David, it exposed a missing heap-based over write
when on the placement new statement.
I've updated the code as per your suggestions, and it now works properly.
> 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
>
>
I will update the patch and regstrap it, so that it is done at once.
I've compared the new test case to a "C" version of it, resulting in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111266
I will attempt to fix it while I'm regstrapping everything else,
I still have 4 patches in queue.
It will give me a brief break from transitioning the tests :)
Thanks for the review,
Benjamin.
