Patch has been updated as per your suggestions and successfully regstrapped
on x86_64-linux-gnu.
call_details::maybe_get_arg_region is now
/* If argument IDX's svalue at the callsite is of pointer type,
return the region it points to.
Otherwise return NULL. */
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);
}
New test is
+
+void test_binop ()
+{
+ char *p = (char *) malloc (4);
+ if (!p)
+ return;
+ int32_t *i = ::new (p + 1) int32_t; /* { dg-warning "heap-based buffer
overflow" } */
+ *i = 42; /* { dg-warning "heap-based buffer overflow" } */
+ free (p);
+}
Is it OK for trunk ?
I didn't resend the whole patch as it otherwise was OK.
Thanks,
Benjamin.
On Fri, Sep 1, 2023 at 12:07 PM Benjamin Priour <priour...@gmail.com> wrote:
> 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.
>
