[libvirt PATCH 3/3] util: bitmap: use g_new0/g_free

[Peter Krempa]

On Tue, Aug 04, 2020 at 23:15:26 -0400, Laine Stump wrote:
> On 8/4/20 5:09 AM, Daniel P. Berrangé wrote:
> > On Mon, Aug 03, 2020 at 07:29:19PM +0200, Peter Krempa wrote:
> > > On Mon, Aug 03, 2020 at 19:18:53 +0200, Ján Tomko wrote:
> > > > Signed-off-by: Ján Tomko <jtomko at redhat.com>
> > > > ---
> > > >   src/util/virbitmap.c | 20 ++++++--------------
> > > >   1 file changed, 6 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> > > > index 4c205016ff..f7dd5d05ad 100644
> > > > --- a/src/util/virbitmap.c
> > > > +++ b/src/util/virbitmap.c
> > > [...]
> > > 
> > > > @@ -126,10 +121,9 @@ virBitmapNewEmpty(void)
> > > >   void
> > > >   virBitmapFree(virBitmapPtr bitmap)
> > > >   {
> > > > -    if (bitmap) {
> > > > -        VIR_FREE(bitmap->map);
> > > > -        VIR_FREE(bitmap);
> > > > -    }
> > > > +    if (bitmap)
> > > > +        g_free(bitmap->map);
> > > Preferrably keep this VIR_FREE or it's expansion to g_clear_pointer. If
> > > a caller uses a stale pointer, it will crash on dereferencing this part.
> > There's no strong reason to do that in a vir*Free() function, since
> > 'bitmap' itself is being freed.
> 
> 
> I think he was pointing out that if you zero out the contents of the
> virBitmap object before you free it, then a caller who mistakenly keeps
> around a pointer to "bitmap" after calling virBitmapFree(bitmap), and then
> attempts to use it, thus causing a dereference of bitmap->map, will get an
> immediate segfault rather than using some chunk of memory that may have
> already been allocated to something else.

Yes, exactly. On the other hand I see that it's mostly pointless. But
please remember that we also cleared the pointer in our own
implementation of VIR_AUTOFREE, which was even more pointless.
Thankfully glib doesn't do such silly things.

> This is a useful concept, but unless we do it *everywhere*, making a special
> case here and there isn't going to have much effect (that was the
> implication of my original response) - it's kind of emptying the ocean with
> a tea spoon. (and also it makes the code uglier and more confusing). Now if
> we could efficiently zero out all blocks of memory as they were freed, then
> maybe there would be some real bug catching value.

I agree. Unfortunately this just clears out pointers, but any other
variables stay the same so other things may broke terribly.

Since the memory can be overwritten right away after being freed any
NULing of memory is not a 100% fix. I still think that it's worth though
in many cases.