Re: [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539]

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2022-08-09 at 23:19 +0200, Tim Lange wrote:
> This patch adds the "shrinks buffer" case to the success_with_move
> modelling of realloc.

Hi Tim, thanks for the patch.

> 
> 2022-08-09  Tim Lange  <mail@tim-lange.me>
> 
> gcc/analyzer/ChangeLog:
> 
>         PR analyzer/106539
>         * region-model-impl-calls.cc
> (region_model::impl_call_realloc):
>         Add get_copied_size function and pass the result as the size
> of the
>         new sized_region.
> 
> ---
>  gcc/analyzer/region-model-impl-calls.cc | 37
> ++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/analyzer/region-model-impl-calls.cc
> b/gcc/analyzer/region-model-impl-calls.cc
> index 8c38e9206fa..50a19a52a21 100644
> --- a/gcc/analyzer/region-model-impl-calls.cc
> +++ b/gcc/analyzer/region-model-impl-calls.cc
> @@ -737,9 +737,11 @@ region_model::impl_call_realloc (const
> call_details &cd)
>                                                   old_size_sval);
>               const svalue *buffer_content_sval
>                 = model->get_store_value (sized_old_reg, cd.get_ctxt
> ());
> +             const svalue *copied_size_sval
> +               = get_copied_size (old_size_sval, new_size_sval);
>               const region *sized_new_reg
>                 = model->m_mgr->get_sized_region (new_reg, NULL,
> -                                                 old_size_sval);
> +                                                 copied_size_sval);

I think that we need to use the same copied size svalue for both
sized_old_reg and sized_new_reg, so that we're using a consistent size
when getting buffer_content_sval.  (I admit my handling of symbolic
sizes of svalues is a bit sloppy, but I'd prefer not to add extra
inconsistencies here)

So the copied_size_sval determination needs to happen before getting
sized_old_reg.

Also, I think renaming sized_{old,new}_reg to copied_{old,new}_reg
might make things clearer (we're copying from a leading subset of the
old region to a leading subset of the new region).


>               model->set_value (sized_new_reg, buffer_content_sval,
>                                 cd.get_ctxt ());
>             }
> @@ -774,6 +776,39 @@ region_model::impl_call_realloc (const
> call_details &cd)
>        else
>         return true;
>      }
> +
> +  private:
> +    /* Return the size svalue for the new region allocated by
> realloc.  */

This comment is misleading - isn't it the size of the existing data to
be copied, i.e. the lesser of the old and new sizes? (allowing for the
fact that one or both could be symbolic, of course)


Please also add a simple explicit test case for the shrinking case
(IIRC from our offlist discussion there's already one that happened to
cover shrinking, but I think that testcase was doing other things too).

Does the patch fix the missing leak warning for the test case in
PR106539?  If so, please have the patch add that to the test suite.


Thanks again
Dave



> +    const svalue *get_copied_size (const svalue *old_size_sval,
> +                                  const svalue *new_size_sval) const
> +    {
> +      tree old_size_cst = old_size_sval->maybe_get_constant ();
> +      tree new_size_cst = new_size_sval->maybe_get_constant ();
> +
> +      if (old_size_cst && new_size_cst)
> +       {
> +         /* Both are constants and comparable.  */
> +         tree cmp = fold_binary (LT_EXPR, boolean_type_node,
> +                                 old_size_cst, new_size_cst);
> +
> +         if (cmp == boolean_true_node)
> +           return old_size_sval;
> +         else
> +           return new_size_sval;
> +       }
> +      else if (new_size_cst)
> +       {
> +         /* OLD_SIZE_SVAL is symbolic, so return that.  */
> +         return old_size_sval;
> +       }
> +      else
> +       {
> +         /* NEW_SIZE_SVAL is symbolic or both are symbolic.
> +            Return NEW_SIZE_SVAL, because implementations of realloc
> +            probably only moves the buffer if the new size is
> larger.  */
> +         return new_size_sval;
> +       }
> +    }
>    };
>  
>    /* Body of region_model::impl_call_realloc.  */