| Message ID | 20170509205242.2237-2-tbsaunde+gcc@tbsaunde.org |
|---|---|
| State | New |
| Headers | show |
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. 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 >
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. 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 > >
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 >> >
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);
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. 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(-)