Re: [PATCH 01/13] improve safety of freeing bitmaps

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, May 10, 2017 at 12:52 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> On Wed, May 10, 2017 at 10:14:17AM +0200, Richard Biener wrote:
>> On Tue, May 9, 2017 at 10:52 PM,  <tbsaunde+gcc@tbsaunde.org> wrote:
>> > From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>
>> >
>> > There's two groups of changes here, first taking a sbitmap &, so that we
>> > can assign null to the pointer after freeing the sbitmap to prevent use
>> > after free through that pointer.  Second we define overloads of
>> > sbitmap_free and bitmap_free taking auto_sbitmap and auto_bitmap
>> > respectively, so that you can't double free the bitmap owned by a
>> > auto_{s,}bitmap.
>>
>> Looks good - but what do you need the void *& overload for?!  That at least
>> needs a comment.
>
> yeah, its gross, I put it in to be compatible with the previous macro.
>   The first problem with removing it is that cfgexpand.c:663 and
>   presumably other places do BITMAP_FREE(bb->aux) which of course
>   depends on being able to pass in a void *.  I'll add a comment and try
>   and look into removing it.

Yeah, please remove it by fixing callers instead.

Richard.

>   Trev
>
>>
>> Richard.
>>
>> > gcc/ChangeLog:
>> >
>> > 2017-05-09  Trevor Saunders  <tbsaunde+gcc@tbsaunde.org>
>> >
>> >         * bitmap.h (BITMAP_FREE): Convert from macro to inline function
>> >         and add overloaded decl for auto_bitmap.
>> >         * sbitmap.h (inline void sbitmap_free): Add overload for
>> >         auto_sbitmap, and change sbitmap to  point to null.
>> > ---
>> >  gcc/bitmap.h  | 21 +++++++++++++++++++--
>> >  gcc/sbitmap.h |  7 ++++++-
>> >  2 files changed, 25 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/gcc/bitmap.h b/gcc/bitmap.h
>> > index f158b447357..7508239cff9 100644
>> > --- a/gcc/bitmap.h
>> > +++ b/gcc/bitmap.h
>> > @@ -129,6 +129,8 @@ along with GCC; see the file COPYING3.  If not see
>> >
>> >  #include "obstack.h"
>> >
>> > +   class auto_bitmap;
>> > +
>> >  /* Bitmap memory usage.  */
>> >  struct bitmap_usage: public mem_usage
>> >  {
>> > @@ -372,8 +374,23 @@ extern hashval_t bitmap_hash (const_bitmap);
>> >  #define BITMAP_GGC_ALLOC() bitmap_gc_alloc ()
>> >
>> >  /* Do any cleanup needed on a bitmap when it is no longer used.  */
>> > -#define BITMAP_FREE(BITMAP) \
>> > -       ((void) (bitmap_obstack_free ((bitmap) BITMAP), (BITMAP) = (bitmap) NULL))
>> > +inline void
>> > +BITMAP_FREE (bitmap &b)
>> > +{
>> > +  bitmap_obstack_free ((bitmap) b);
>> > +  b = NULL;
>> > +}
>> > +
>> > +inline void
>> > +BITMAP_FREE (void *&b)
>> > +{
>> > +  bitmap_obstack_free ((bitmap) b);
>> > +  b = NULL;
>> > +}
>> > +
>> > +/* Intentionally unimplemented to ensure it is never called with an
>> > +   auto_bitmap argument.  */
>> > +void BITMAP_FREE (auto_bitmap);
>> >
>> >  /* Iterator for bitmaps.  */
>> >
>> > diff --git a/gcc/sbitmap.h b/gcc/sbitmap.h
>> > index ce4d27d927c..cba0452cdb9 100644
>> > --- a/gcc/sbitmap.h
>> > +++ b/gcc/sbitmap.h
>> > @@ -82,6 +82,8 @@ along with GCC; see the file COPYING3.  If not see
>> >  #define SBITMAP_ELT_BITS (HOST_BITS_PER_WIDEST_FAST_INT * 1u)
>> >  #define SBITMAP_ELT_TYPE unsigned HOST_WIDEST_FAST_INT
>> >
>> > +class auto_sbitmap;
>> > +
>> >  struct simple_bitmap_def
>> >  {
>> >    unsigned int n_bits;         /* Number of bits.  */
>> > @@ -208,11 +210,14 @@ bmp_iter_next (sbitmap_iterator *i, unsigned *bit_no ATTRIBUTE_UNUSED)
>> >         bmp_iter_next (&(ITER), &(BITNUM)))
>> >  #endif
>> >
>> > -inline void sbitmap_free (sbitmap map)
>> > +inline void sbitmap_free (sbitmap &map)
>> >  {
>> >    free (map);
>> > +  map = NULL;
>> >  }
>> >
>> > +void sbitmap_free (auto_sbitmap);
>> > +
>> >  inline void sbitmap_vector_free (sbitmap * vec)
>> >  {
>> >    free (vec);
>> > --
>> > 2.11.0
>> >