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.