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.

Reply via email to